diff --git a/public/channels.js b/public/channels.js index 964bfa876..d82ebcf60 100644 --- a/public/channels.js +++ b/public/channels.js @@ -55,6 +55,53 @@ // can mutate freely without leaking changes back to the input. return survivors.length ? restMsgs.concat(survivors) : restMsgs.slice(); } + // #2095 — loadChannels() replaces `channels` with the server snapshot, and + // the snapshot knows nothing about state that only ever lived in this tab: + // unread counts, and activity the WS handler applied while the request was + // in flight. mergeWsAppendedIntoRest above does the same job for `messages` + // (#1498); this is its counterpart for `channels`. + // + // Deliberately enriches ONLY rows the snapshot already contains. Carrying a + // missing row over would resurrect channels the region filter just excluded, + // which is a worse bug than the one being fixed. A channel genuinely dropped + // by a race re-appears on its next packet. + // + // Returns a fresh array of fresh objects; never aliases or mutates an input. + function mergeClientChannelState(freshChannels, prevChannels) { + if (!Array.isArray(freshChannels)) return []; + if (!Array.isArray(prevChannels) || prevChannels.length === 0) { + return freshChannels.map(function (c) { return Object.assign({}, c); }); + } + var prevByHash = new Map(); + for (var i = 0; i < prevChannels.length; i++) { + var p = prevChannels[i]; + if (p && p.hash) prevByHash.set(p.hash, p); + } + return freshChannels.map(function (c) { + var out = Object.assign({}, c); + var prev = out.hash ? prevByHash.get(out.hash) : null; + if (!prev) return out; + // Unread is counted in this tab and exists nowhere else. Carried when + // the property is present, including an explicit 0: dropping that would + // leave the row with undefined, which is a different thing from "read". + if (Object.prototype.hasOwnProperty.call(prev, 'unread')) out.unread = prev.unread; + // The user's own marks, re-derived from storage by mergeUserChannels() + // straight after this, but carried here so a row is never briefly wrong. + if (prev.userAdded) out.userAdded = true; + if (prev.userLabel) out.userLabel = prev.userLabel; + // A WS batch that landed while the request was in flight is newer than + // the snapshot. Keep the whole set together: a sender without its + // message reads as a different message. + if ((prev.lastActivityMs || 0) > (out.lastActivityMs || 0)) { + out.lastActivityMs = prev.lastActivityMs; + out.lastSender = prev.lastSender; + out.lastMessage = prev.lastMessage; + if ((prev.messageCount || 0) > (out.messageCount || 0)) out.messageCount = prev.messageCount; + } + return out; + }); + } + let autoScroll = true; let nodeCache = {}; let selectedNode = null; @@ -1662,10 +1709,18 @@ if (showEnc) params.push('includeEncrypted=true'); const qs = params.length ? '?' + params.join('&') : ''; const data = await api('/channels' + qs, { ttl: CLIENT_TTL.channels }); - channels = (data.channels || []).map(ch => { + const fresh = (data.channels || []).map(ch => { ch.lastActivityMs = ch.lastActivity ? new Date(ch.lastActivity).getTime() : 0; return ch; - }).sort((a, b) => (b.lastActivityMs || 0) - (a.lastActivityMs || 0)); + }); + // #2095 — carry client-only state across the replacement, then re-derive + // the user's PSK rows from storage. Both must happen BEFORE + // reconcileSelectionAfterChannelRefresh(), which evicts the selection + // when it cannot find selectedHash: a user:* hash is never in the server + // snapshot, so without this a refresh closed the open conversation. + channels = mergeClientChannelState(fresh, channels) + .sort((a, b) => (b.lastActivityMs || 0) - (a.lastActivityMs || 0)); + if (typeof ChannelDecrypt !== 'undefined' && ChannelDecrypt) mergeUserChannels(); renderChannelList(); reconcileSelectionAfterChannelRefresh(); } catch (e) { @@ -2316,6 +2371,7 @@ window._channelsSelectChannelForTest = selectChannel; window._channelsRefreshMessagesForTest = refreshMessages; window._channelsMergeWsAppendedIntoRestForTest = mergeWsAppendedIntoRest; + window._channelsMergeClientChannelStateForTest = mergeClientChannelState; window._channelsLoadChannelsForTest = loadChannels; window._channelsBeginMessageRequestForTest = beginMessageRequest; window._channelsIsStaleMessageRequestForTest = isStaleMessageRequest; diff --git a/test-all.sh b/test-all.sh index 889327026..5d8a8e416 100755 --- a/test-all.sh +++ b/test-all.sh @@ -139,6 +139,7 @@ node tests/unit/test-issue-1997-distance-building.js node tests/unit/test-issue-2042-recent-adverts-label.js node tests/unit/test-issue-2001-map-scope-state.js node tests/unit/test-issue-2012-clear-filters-selection.js +node tests/unit/test-issue-2095-channels-client-state.js node tests/unit/test-live-anims.js node tests/unit/test-live-dt-cap-1524.js node tests/unit/test-live-legend-helper.js diff --git a/tests/unit/test-issue-2095-channels-client-state.js b/tests/unit/test-issue-2095-channels-client-state.js new file mode 100644 index 000000000..0ec984250 --- /dev/null +++ b/tests/unit/test-issue-2095-channels-client-state.js @@ -0,0 +1,236 @@ +/** + * #2095 — loadChannels() replaced the channel array and discarded every + * client-only field on it. + * + * Two consequences, one root cause. The array assignment at loadChannels() + * carries nothing across, and mergeUserChannels() only ran from init(), so a + * region-filter change or the show-encrypted toggle destroyed the My Channels + * section, every unread badge, and the user's own labels — and evicted the + * selected channel if it was a PSK row, closing the open conversation. + * + * Part 1 tests the pure helper directly. Part 2 drives loadChannels() through + * its test hook with a stubbed api(), which is what proves the helper is + * actually wired in rather than merely present. + * + * Sandbox pattern copied from test-channels-merge-1498-unit.js. + */ +'use strict'; +const vm = require('vm'); +const fs = require('fs'); +const assert = require('assert'); + +const noop = () => {}; +const fakeEl = { + addEventListener: noop, removeEventListener: noop, querySelector: () => fakeEl, + querySelectorAll: () => [], classList: { add: noop, remove: noop, toggle: noop, contains: () => false }, + appendChild: noop, removeChild: noop, setAttribute: noop, getAttribute: () => null, + textContent: '', innerHTML: '', style: {}, dataset: {}, scrollTop: 0, scrollHeight: 0, +}; +const doc = { + readyState: 'complete', createElement: () => ({ ...fakeEl }), head: fakeEl, body: fakeEl, + getElementById: () => null, querySelector: () => null, querySelectorAll: () => [], + addEventListener: noop, removeEventListener: noop, +}; + +// Stored PSK keys the user added. mergeUserChannels() reads these, so they are +// what makes the My Channels section exist at all. +let storedKeys = {}; +let storedLabels = {}; + +const ctx = { + window: { addEventListener: noop, removeEventListener: noop }, + document: doc, console, Date, Math, JSON, Set, Map, Array, Object, Promise, + Response: function () {}, Error, Number, String, Boolean, isNaN, parseInt, parseFloat, + setTimeout, clearTimeout, setInterval, clearInterval, + history: { replaceState: noop, pushState: noop }, + location: { hash: '', href: '', pathname: '/' }, + navigator: { userAgent: 'node' }, + localStorage: { getItem: () => null, setItem: noop, removeItem: noop }, + RegionFilter: { getRegionParam: () => '', onChange: () => noop }, + CLIENT_TTL: { channels: 15000 }, + ChannelDecrypt: { + getStoredKeys: () => storedKeys, + getLabels: () => storedLabels, + }, + truncate: (s) => s, + formatHashHex: (h) => String(h), + channelDisplayName: (c) => (c && (c.userLabel || c.name)) || '', + escapeHtml: (s) => String(s), + getSenderColor: () => '#000', + registerPage: noop, + fetch: () => Promise.resolve({ json: () => Promise.resolve({}) }), +}; + +// The server snapshot loadChannels() will receive. Reassigned per test. +let apiChannels = []; +ctx.api = () => Promise.resolve({ channels: apiChannels.map(c => ({ ...c })) }); + +vm.createContext(ctx); +try { + vm.runInContext(fs.readFileSync('public/channels.js', 'utf8'), ctx); +} catch (e) { + // The IIFE may throw on missing DOM further down; the hooks we need are + // exported before that point. +} + +const merge = ctx.window._channelsMergeClientChannelStateForTest; +const loadChannels = ctx.window._channelsLoadChannelsForTest; +const setState = ctx.window._channelsSetStateForTest; +const getState = ctx.window._channelsGetStateForTest; + +for (const [name, fn] of Object.entries({ merge, loadChannels, setState, getState })) { + if (typeof fn !== 'function') { + console.error(`FATAL: ${name} not exported by channels.js`); + process.exit(2); + } +} + +let passed = 0, failed = 0; +function test(name, fn) { + try { fn(); console.log(` PASS ${name}`); passed++; } + catch (e) { console.log(` FAIL ${name}\n ${e.message}`); failed++; } +} +async function atest(name, fn) { + try { await fn(); console.log(` PASS ${name}`); passed++; } + catch (e) { console.log(` FAIL ${name}\n ${e.message}`); failed++; } +} + +console.log('\n=== #2095 part 1: mergeClientChannelState() ==='); + +test('carries unread across the replacement', () => { + const prev = [{ hash: 'a', unread: 3 }, { hash: 'b', unread: 0 }]; + const fresh = [{ hash: 'a' }, { hash: 'b' }]; + const out = merge(fresh, prev); + assert.strictEqual(out[0].unread, 3, 'unread on a was dropped'); + assert.strictEqual(out[1].unread, 0); +}); + +test('keeps WS-fresher activity fields when the snapshot is older', () => { + // The WS handler already applied a newer message before the REST snapshot + // landed. Reverting to the snapshot is the flicker in #2095 finding 2. + const prev = [{ hash: 'a', lastActivityMs: 2000, lastSender: 'ON8AR', lastMessage: 'new', messageCount: 8 }]; + const fresh = [{ hash: 'a', lastActivityMs: 1000, lastSender: 'OLD', lastMessage: 'old', messageCount: 7 }]; + const out = merge(fresh, prev); + assert.strictEqual(out[0].lastActivityMs, 2000); + assert.strictEqual(out[0].lastSender, 'ON8AR'); + assert.strictEqual(out[0].lastMessage, 'new'); + assert.strictEqual(out[0].messageCount, 8); +}); + +test('the server wins when its snapshot is the newer one', () => { + const prev = [{ hash: 'a', lastActivityMs: 1000, lastSender: 'STALE', lastMessage: 'stale', messageCount: 2 }]; + const fresh = [{ hash: 'a', lastActivityMs: 5000, lastSender: 'FRESH', lastMessage: 'fresh', messageCount: 9 }]; + const out = merge(fresh, prev); + assert.strictEqual(out[0].lastActivityMs, 5000); + assert.strictEqual(out[0].lastSender, 'FRESH'); + assert.strictEqual(out[0].messageCount, 9); +}); + +test('does not resurrect a channel the snapshot left out', () => { + // A region-filter change legitimately narrows the list. Carrying survivors + // over would defeat the filter, which is why this helper only enriches rows + // that are already in the fresh snapshot. + const prev = [{ hash: 'a', unread: 1 }, { hash: 'gone', unread: 9 }]; + const fresh = [{ hash: 'a' }]; + const out = merge(fresh, prev); + assert.strictEqual(out.length, 1, 'a filtered-out channel came back'); + assert.strictEqual(out[0].hash, 'a'); +}); + +test('never aliases or mutates its inputs', () => { + const prev = [{ hash: 'a', unread: 4 }]; + const fresh = [{ hash: 'a' }]; + const out = merge(fresh, prev); + assert.notStrictEqual(out, fresh, 'returned the input array itself'); + out[0].unread = 99; + assert.strictEqual(prev[0].unread, 4, 'mutating the result reached prev'); +}); + +test('tolerates empty and missing inputs', () => { + // Not deepStrictEqual: an array built inside the vm realm has a different + // Array.prototype, which that assertion compares and rejects. + assert.strictEqual(merge([], []).length, 0); + assert.strictEqual(merge(null, null).length, 0); + assert.strictEqual(merge([{ hash: 'a' }], null).length, 1); + assert.strictEqual(merge([{ hash: 'a' }], undefined).length, 1); +}); + +test('ignores rows with no hash rather than matching them together', () => { + const prev = [{ unread: 5 }, { hash: '', unread: 6 }]; + const fresh = [{ hash: 'a' }, { hash: '' }]; + const out = merge(fresh, prev); + assert.ok(!out[0].unread, 'a hashless prev row leaked onto a real channel'); +}); + +console.log('\n=== #2095 part 2: loadChannels() keeps client state ==='); + +(async function () { + await atest('a refresh keeps the My Channels rows', async () => { + // The bug: the user has a PSK channel, the server does not know it, and any + // refresh replaced the array with the server snapshot, so the row vanished. + storedKeys = { 'MyPSK': 'deadbeef' }; + storedLabels = { 'MyPSK': 'Mijn kanaal' }; + apiChannels = [{ hash: 'public1', name: 'public1', lastActivity: null }]; + setState({ channels: [], messages: [], selectedHash: null }); + + await loadChannels(true); + + const after = getState().channels; + const mine = after.filter(c => c.userAdded === true); + assert.strictEqual(mine.length, 1, `My Channels lost on refresh (got ${JSON.stringify(after.map(c => c.hash))})`); + assert.strictEqual(mine[0].hash, 'user:MyPSK'); + assert.strictEqual(mine[0].userLabel, 'Mijn kanaal', 'the user label was dropped'); + }); + + await atest('a refresh keeps unread badges', async () => { + storedKeys = {}; + storedLabels = {}; + apiChannels = [{ hash: 'public1', name: 'public1', lastActivity: null }]; + setState({ channels: [{ hash: 'public1', name: 'public1', unread: 7 }], messages: [], selectedHash: null }); + + await loadChannels(true); + + const ch = getState().channels.find(c => c.hash === 'public1'); + assert.ok(ch, 'the channel disappeared'); + assert.strictEqual(ch.unread, 7, 'the unread badge reset to 0 on refresh'); + }); + + await atest('a refresh does not close an open PSK conversation', async () => { + // The worst of the two: reconcileSelectionAfterChannelRefresh() did not + // find the user:* hash in the server snapshot, so it nulled the selection, + // emptied messages and rewrote the URL. + storedKeys = { 'MyPSK': 'deadbeef' }; + storedLabels = {}; + apiChannels = [{ hash: 'public1', name: 'public1', lastActivity: null }]; + setState({ + channels: [{ hash: 'user:MyPSK', name: 'MyPSK', userAdded: true }], + messages: [{ text: 'hello' }], + selectedHash: 'user:MyPSK', + }); + + await loadChannels(true); + + const st = getState(); + assert.strictEqual(st.selectedHash, 'user:MyPSK', 'the open PSK channel was deselected'); + assert.strictEqual(st.messages.length, 1, 'the open conversation was emptied'); + }); + + await atest('a refresh still drops a channel the server filtered out', async () => { + // The fix must not defeat the region filter. + storedKeys = {}; + storedLabels = {}; + apiChannels = [{ hash: 'in-region', name: 'in-region', lastActivity: null }]; + setState({ + channels: [{ hash: 'in-region', name: 'in-region' }, { hash: 'out-of-region', name: 'out-of-region' }], + messages: [], selectedHash: null, + }); + + await loadChannels(true); + + const hashes = getState().channels.map(c => c.hash); + assert.ok(!hashes.includes('out-of-region'), `the filter was defeated: ${JSON.stringify(hashes)}`); + }); + + console.log(`\n${passed} passed, ${failed} failed`); + process.exit(failed ? 1 : 0); +})();