diff --git a/ACCEPTANCE.md b/ACCEPTANCE.md index 1dcab2e2..a1265ea6 100644 --- a/ACCEPTANCE.md +++ b/ACCEPTANCE.md @@ -2341,3 +2341,92 @@ idempotent, `user_version`-gated), **FC-A2** (seed-then-delta — the warm delta contract is unchanged; only the backfill anchor moved), **PL-A1/PL-A2** (progressive load), and **B1**. No POST/GET/event contract change and no ingestor change, so **C2**, `CONTRACTS.md`, and the Python suite are unaffected. + +--- + +## Bugfix: Chat-log entry retention, advert suppression, and chat vertical scroll + +Three independent chat-panel defects, all frontend-only (no API/DB/ingestor +change, so the apex (I) and privacy (II) invariants are untouched): + +**(A1)** `rebuildNodeDerivedState` stored the *aggregated* snapshot arrays back +into the raw accumulators (`allTelemetryEntries` / `allPositionEntries` / +`allNeighbors`), which are also the merge targets for every refresh + backfill +page. Re-aggregating an already-aggregated array is lossy (`aggregateSnapshots` +clones with `{...snapshot}`, dropping the non-enumerable `snapshots` history, and +merges oldest-last so the stalest reading's `rx_time`/`id` win), collapsing each +node's history to `{stale-first, newest}` — so a telemetry/position Log entry +appeared for one refresh tick and vanished on the next (no scrolling involved). +The accumulators now stay **raw**; the aggregated forms are locals used only to +enrich node records, so every packet keeps a stable, id-keyed Log entry. + +**(A2)** The advert-suppression claim key folded in `node_num` and required BOTH +`node_id` and `node_num` to match. Specific events (telemetry/positions/ +neighbors) frequently carry only `node_id` (`node_num` is int|nil per CONTRACTS, +commonly nil for MeshCore) while the node record carries a `node_num`, so the +combined key failed to match and a redundant "Updated node info (advert)" line +leaked alongside the specific entry (violating LV7/LV-A7). Suppression now keys on +the canonical `!%08x` id alone (which `normaliseNodeId` derives from `node_num` +when needed), so the id identifies a node across every event shape. + +**(B)** Every chat render force-scrolled the active panel to the bottom (in +`setActiveTab`, plus a second `scrollActiveChatPanelToBottom` call), so a live +update (40-80/hr in production) yanked the reader back to the bottom and made +upward scrolling impossible. The prior LD-A2 fix preserved only the *horizontal* +tab-list scroll. `renderChatTabs` now captures the active panel's vertical +`scrollTop` before the subtree rebuild and restores it. + +### CL-A1 -- telemetry/position Log entries survive successive refreshes +```bash +( cd web && node --test public/assets/js/app/__tests__/main-log-snapshot-retention.test.js ) +``` +**Expected:** pass. After one node emits three telemetry packets across three +refreshes, all three stay loaded (`getLoadedTelemetryCount() === 3`) and the +rendered Log shows all three "Broadcasted telemetry" entries — the raw +accumulator is no longer collapsed to a single per-node aggregate by the next +tick's re-aggregation. + +### CL-A2 -- the advert is suppressed when a specific event omits `node_num`, and for encrypted-message hears +```bash +( cd web && node --test public/assets/js/app/__tests__/chat-log-tabs.test.js ) +``` +**Expected:** pass. When the node record carries a `node_num` but the telemetry/ +position rows carry only `node_id`, `buildChatTabModel(...).logEntries` still +emits the telemetry and position entries and **no** redundant node-info (advert) +entry. An id-less heard (no `node_id`, no derivable `node_num`) claims nothing and +is never suppressed. An **encrypted message** (in either the `messages` or the +`logOnlyMessages` feed) claims its sender's heard, so a node heard only via a +`🔒 encrypted message on channel ` line shows **no** redundant +`Updated node info (advert)` beneath it — the encrypted-message line is that +heard's Log representation, mirroring how a decrypted message becomes a +`(message)` node-info. Realises LV-A7 ("a position/telemetry/.../message +suppresses a redundant advert line") across the `node_num`-nil and encrypted- +message shapes that previously slipped through. + +### CL-A3 -- a passive chat re-render preserves the reader's vertical scroll +```bash +( cd web && node --test public/assets/js/app/__tests__/chat-tabs.test.js ) +``` +**Expected:** pass. `renderChatTabs` captures the active panel's `scrollTop` +before the `replaceChildren` rebuild and restores it on the fresh panel: a reader +scrolled up keeps their exact offset across a passive refresh, a bottom-pinned +reader stays pinned to the new bottom (tail-follow), and an initial render (no +prior panel) pins to the bottom. The per-render force-scroll (and the redundant +`scrollActiveChatPanelToBottom`) are gone; panel scroll-to-bottom now fires only +on an explicit tab switch (click/dropdown). Composes with the LD-A2 horizontal +scroll preservation and the LV8 dropdown. + +### CL-R1 -- Regression: prior acceptance still holds +```bash +( cd web && npm test ) && ( cd web && bundle exec rspec ) +( . .venv/bin/activate && pytest -q tests/ ) +``` +**Expected:** all green. At risk and explicitly required to stay green: **LV-A7** +(node-centric Log; the advert-suppression rule is strengthened, not weakened), +**LD-A2** (horizontal tab scroll still preserved -- the new vertical-scroll +preservation composes with it), **LV-A8** (the channel dropdown still jumps tabs), +**VF-A6 / CR-A1** (an idle re-render still materialises 0 entries -- the scroll +capture touches only already-built DOM), **CB-A1** (every bulk collection still +backfills; the accumulators it merges into are raw, which is the shape the model +already expects), and **B1** (all suites). Frontend-only: no POST/GET/event +contract change, so `CONTRACTS.md` and the Python suite are unaffected. diff --git a/web/public/assets/js/app/__tests__/chat-log-tabs.test.js b/web/public/assets/js/app/__tests__/chat-log-tabs.test.js index f34d0a27..b1ffb3ce 100644 --- a/web/public/assets/js/app/__tests__/chat-log-tabs.test.js +++ b/web/public/assets/js/app/__tests__/chat-log-tabs.test.js @@ -259,6 +259,71 @@ test('buildChatTabModel does not emit redundant node-info for telemetry/neighbor ); }); +test('buildChatTabModel suppresses the advert when the specific event omits node_num (A2)', () => { + // Real-world shape: the node record carries a node_num, but the telemetry / + // position rows carry only node_id (node_num is int|nil per CONTRACTS, and is + // frequently nil — notably for MeshCore). The heard is the same node + ts, so + // the specific events must still claim it and suppress the redundant advert. + const model = buildChatTabModel({ + nodes: [{ node_id: '!a', node_num: 10, last_heard: NOW - 5 }], + telemetry: [{ node_id: '!a', rx_time: NOW - 5, battery_level: 80 }], + positions: [{ node_id: '!a', rx_time: NOW - 5, latitude: 1, longitude: 2 }], + nowSeconds: NOW, + windowSeconds: WINDOW + }); + // The specific events still render their own entries... + assert.ok(model.logEntries.some(e => e.type === CHAT_LOG_ENTRY_TYPES.TELEMETRY)); + assert.ok(model.logEntries.some(e => e.type === CHAT_LOG_ENTRY_TYPES.POSITION)); + // ...and no redundant "Updated node info (advert)" appears despite the + // node_num mismatch between the node record and the telemetry/position rows. + assert.equal( + model.logEntries.filter(e => e.type === CHAT_LOG_ENTRY_TYPES.NODE_INFO).length, + 0 + ); +}); + +test('buildChatTabModel suppresses the advert when a node was heard via an encrypted message (A3, logOnly feed)', () => { + // Production shape: encrypted messages arrive via logOnlyMessages. The + // "🔒 encrypted message" line is the sender's Log representation for that heard, + // so the redundant "Updated node info (advert)" must be suppressed (LV7). + const model = buildChatTabModel({ + nodes: [{ node_id: '!a', last_heard: NOW - 5 }], + logOnlyMessages: [{ id: 'e1', encrypted: true, from_id: '!a', channel: 78, rx_time: NOW - 5 }], + nowSeconds: NOW, + windowSeconds: WINDOW + }); + assert.ok(model.logEntries.some(e => e.type === CHAT_LOG_ENTRY_TYPES.MESSAGE_ENCRYPTED)); + assert.equal(model.logEntries.filter(e => e.type === CHAT_LOG_ENTRY_TYPES.NODE_INFO).length, 0); +}); + +test('buildChatTabModel suppresses the advert for an encrypted message in the messages feed (A3)', () => { + const model = buildChatTabModel({ + nodes: [{ node_id: '!a', last_heard: NOW - 5 }], + messages: [{ id: 'e2', encrypted: true, from_id: '!a', channel: 78, rx_time: NOW - 5 }], + nowSeconds: NOW, + windowSeconds: WINDOW + }); + assert.ok(model.logEntries.some(e => e.type === CHAT_LOG_ENTRY_TYPES.MESSAGE_ENCRYPTED)); + assert.equal(model.logEntries.filter(e => e.type === CHAT_LOG_ENTRY_TYPES.NODE_INFO).length, 0); +}); + +test('buildChatTabModel keeps an id-less heard from claiming or being suppressed (A2 edge)', () => { + const model = buildChatTabModel({ + // A node heard with neither node_id nor node_num: its advert still renders, + // and the unresolved (null) id is never used to suppress another heard. + nodes: [{ last_heard: NOW - 5 }], + // A telemetry row with no node_id/node_num: it renders its own entry but + // claims nothing — an id-less heard cannot stand in for any node's advert. + telemetry: [{ rx_time: NOW - 6, battery_level: 1 }], + nowSeconds: NOW, + windowSeconds: WINDOW + }); + assert.ok(model.logEntries.some(e => e.type === CHAT_LOG_ENTRY_TYPES.TELEMETRY)); + const adverts = model.logEntries.filter(e => e.type === CHAT_LOG_ENTRY_TYPES.NODE_INFO); + assert.equal(adverts.length, 1); + assert.equal(adverts[0].reason, 'advert'); +}); + test('buildChatTabModel attributes a message whose sender is absent from the nodes feed', () => { const model = buildChatTabModel({ nodes: [], diff --git a/web/public/assets/js/app/__tests__/chat-tabs.test.js b/web/public/assets/js/app/__tests__/chat-tabs.test.js index 642e54f9..c578371b 100644 --- a/web/public/assets/js/app/__tests__/chat-tabs.test.js +++ b/web/public/assets/js/app/__tests__/chat-tabs.test.js @@ -17,7 +17,7 @@ import test from 'node:test'; import assert from 'node:assert/strict'; -import { renderChatTabs } from '../chat-tabs.js'; +import { renderChatTabs, __test__ } from '../chat-tabs.js'; class MockClassList { constructor() { @@ -391,6 +391,114 @@ test('renderChatTabs does not scroll the active tab into view on a passive re-re }); +/** Return the single visible (active) panel within a rendered container. */ +function activePanel(container) { + const panelWrapper = container.children[1]; + if (!panelWrapper || !Array.isArray(panelWrapper.children)) return null; + return panelWrapper.children.find(panel => panel && panel.hidden === false) || null; +} + +test('renderChatTabs preserves the active panel vertical scroll across a passive re-render (bugfix B)', () => { + const document = createMockDocument(); + const container = new MockElement('div'); + const tabs = () => [ + { id: 'log', label: 'Log', content: new MockElement('div') }, + { id: 'c0', label: 'Default', content: new MockElement('div') } + ]; + renderChatTabs({ document, container, tabs: tabs(), defaultActiveTabId: 'c0' }); + + // The user scrolls up to read history: the panel is no longer at the bottom. + const panel1 = activePanel(container); + panel1.scrollHeight = 1000; + panel1.clientHeight = 300; + panel1.scrollTop = 120; // 1000 - 120 - 300 = 580 px from the bottom → not pinned + + // A passive live refresh (no tab switch) re-renders the same tabs. + renderChatTabs({ document, container, tabs: tabs(), defaultActiveTabId: 'c0' }); + const panel2 = activePanel(container); + + assert.notEqual(panel2, panel1); // a fresh panel element (full subtree rebuild) + assert.equal( + panel2.scrollTop, + 120, + 'a passive re-render must preserve the vertical scroll, not yank the reader to the bottom' + ); +}); + +test('renderChatTabs keeps a bottom-pinned reader pinned across a passive re-render (tail-follow, bugfix B)', () => { + const document = createMockDocument(); + const container = new MockElement('div'); + const tabs = () => [ + { id: 'log', label: 'Log', content: new MockElement('div') }, + { id: 'c0', label: 'Default', content: new MockElement('div') } + ]; + renderChatTabs({ document, container, tabs: tabs(), defaultActiveTabId: 'c0' }); + + // The user is at the bottom, reading the newest entries. + const panel1 = activePanel(container); + panel1.scrollHeight = 1000; + panel1.clientHeight = 300; + panel1.scrollTop = 700; // 1000 - 700 - 300 = 0 → pinned to the bottom + + renderChatTabs({ document, container, tabs: tabs(), defaultActiveTabId: 'c0' }); + const panel2 = activePanel(container); + + // Still pinned: scrolled to the new bottom so freshly-arrived entries stay visible. + assert.equal( + panel2.scrollTop, + panel2.scrollHeight, + 'a bottom-pinned reader must stay pinned to the new bottom (tail-follow)' + ); +}); + +test('capturePreviousActivePanelScroll returns null when there is no prior panel (bugfix B)', () => { + const { capturePreviousActivePanelScroll } = __test__; + assert.equal(capturePreviousActivePanelScroll(null), null); + assert.equal(capturePreviousActivePanelScroll({}), null); + assert.equal(capturePreviousActivePanelScroll({ children: [] }), null); +}); + +test('capturePreviousActivePanelScroll returns null when every panel is hidden (bugfix B)', () => { + const { capturePreviousActivePanelScroll } = __test__; + const container = { children: [{}, { children: [null, { hidden: true }, { hidden: true }] }] }; + assert.equal(capturePreviousActivePanelScroll(container), null); +}); + +test('capturePreviousActivePanelScroll reports the visible panel offset and pinned state (bugfix B)', () => { + const { capturePreviousActivePanelScroll, SCROLL_PIN_TOLERANCE_PX } = __test__; + assert.equal(SCROLL_PIN_TOLERANCE_PX, 4); + + // A hidden panel precedes the visible one (mirrors the Log tab before a channel). + const pinnedPanel = { hidden: false, scrollTop: 700, scrollHeight: 1000, clientHeight: 300 }; + const pinned = capturePreviousActivePanelScroll({ children: [{}, { children: [{ hidden: true }, pinnedPanel] }] }); + assert.deepEqual(pinned, { top: 700, pinned: true }); // 1000 - 700 - 300 = 0 <= tol + + const scrolledUp = { hidden: false, scrollTop: 100, scrollHeight: 1000, clientHeight: 300 }; + const up = capturePreviousActivePanelScroll({ children: [{}, { children: [scrolledUp] }] }); + assert.deepEqual(up, { top: 100, pinned: false }); // 600 px from the bottom > tol +}); + +test('applyActivePanelScroll pins, preserves, or no-ops on a missing panel (bugfix B)', () => { + const { applyActivePanelScroll } = __test__; + // No panel → no throw, nothing to do. + assert.doesNotThrow(() => applyActivePanelScroll(null, { top: 5, pinned: false })); + + // No prior state (initial render) → pin to the bottom. + const initial = { scrollTop: 0, scrollHeight: 900 }; + applyActivePanelScroll(initial, null); + assert.equal(initial.scrollTop, 900); + + // Was pinned → pin to the new bottom (tail-follow). + const pinned = { scrollTop: 0, scrollHeight: 900 }; + applyActivePanelScroll(pinned, { top: 42, pinned: true }); + assert.equal(pinned.scrollTop, 900); + + // Was scrolled up → restore the exact offset. + const preserved = { scrollTop: 0, scrollHeight: 900 }; + applyActivePanelScroll(preserved, { top: 42, pinned: false }); + assert.equal(preserved.scrollTop, 42); +}); + test('renderChatTabs renders a channel dropdown selector that jumps to a tab (LV8)', () => { const document = createMockDocument(); const container = new MockElement('div'); diff --git a/web/public/assets/js/app/__tests__/main-log-snapshot-retention.test.js b/web/public/assets/js/app/__tests__/main-log-snapshot-retention.test.js new file mode 100644 index 00000000..6b3af39e --- /dev/null +++ b/web/public/assets/js/app/__tests__/main-log-snapshot-retention.test.js @@ -0,0 +1,135 @@ +/* + * Copyright © 2025-26 l5yth & contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * Regression guard for the disappearing-log-entry bug (bugfix A1). + * + * ``rebuildNodeDerivedState`` used to store the *aggregated* snapshot arrays + * back into the raw accumulators (``allTelemetryEntries`` / ``allPositionEntries`` + * / ``allNeighbors``), and those same variables are the merge targets for every + * refresh. Re-aggregating an already-aggregated array is lossy + * (``aggregateSnapshots`` clones with ``{...snapshot}``, dropping the + * non-enumerable ``snapshots`` history, and merges oldest-last so the stalest + * reading's ``rx_time`` wins), so each per-node aggregate collapsed to + * ``{stale-first, newest}``. The visible effect: a telemetry/position log entry + * appeared for one refresh tick and then vanished on the next — no scrolling + * involved. The accumulators must stay raw so every packet keeps a stable, + * id-keyed Log entry across refreshes. + * + * @module app/__tests__/main-log-snapshot-retention + */ + +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { createDomEnvironment } from './dom-environment.js'; +import { initializeApp } from '../main.js'; + +/** Config with the auto-refresh timer off so the test drives every tick. */ +const CONFIG = Object.freeze({ + channel: 'Primary', + frequency: '915MHz', + refreshMs: 0, + refreshIntervalSeconds: 0, + chatEnabled: true, + mapCenter: { lat: 0, lon: 0 }, + mapZoom: null, + maxDistanceKm: 0, + instancesFeatureEnabled: false, + instanceDomain: null, + snapshotWindowSeconds: 3600, +}); + +const NOW = Math.floor(Date.now() / 1000); +const NODE_ID = '!00000001'; + +/** Yield to pending microtasks/timers so fire-and-forget work (stats) settles. */ +function settle(ms = 60) { + return new Promise(r => setTimeout(r, ms)); +} + +/** Count non-overlapping occurrences of ``needle`` in ``haystack``. */ +function countOccurrences(haystack, needle) { + let count = 0; + let from = 0; + for (;;) { + const idx = haystack.indexOf(needle, from); + if (idx === -1) break; + count += 1; + from = idx + needle.length; + } + return count; +} + +test('telemetry log entries survive successive refreshes (A1: no re-aggregation erosion)', async () => { + // One node emits a fresh telemetry packet on each tick; all three timestamps + // are within the recent window and must remain represented in the Log. + let telemetryBody = [ + { id: 1, node_id: NODE_ID, node_num: 1, rx_time: NOW - 60, battery_level: 80 }, + ]; + const nodeBody = [ + { node_id: NODE_ID, node_num: 1, short_name: 'N1', long_name: 'Node One', role: 'CLIENT', last_heard: NOW }, + ]; + + function stubFetch(url) { + if (url.startsWith('/api/nodes/')) return jsonResponse(null); + if (url.startsWith('/api/nodes')) return jsonResponse(nodeBody); + if (url.startsWith('/api/telemetry')) return jsonResponse(telemetryBody); + return jsonResponse([]); // messages/positions/neighbors/traces/stats + } + + const env = createDomEnvironment({ includeBody: true }); + env.registerElement('chat', env.createElement('div', 'chat')); + const originalFetch = globalThis.fetch; + globalThis.fetch = url => stubFetch(url); + try { + const { _testUtils } = initializeApp(CONFIG); + await _testUtils.initialLoad; + await settle(); + + // Two further refreshes, each delivering one new telemetry packet. + telemetryBody = [{ id: 2, node_id: NODE_ID, node_num: 1, rx_time: NOW - 40, battery_level: 81 }]; + await _testUtils.refresh(); + await settle(); + telemetryBody = [{ id: 3, node_id: NODE_ID, node_num: 1, rx_time: NOW - 20, battery_level: 82 }]; + await _testUtils.refresh(); + await settle(); + + // The raw accumulator must retain all three packets (the bug collapsed it to 1). + assert.equal( + _testUtils.getLoadedTelemetryCount(), + 3, + `all three telemetry packets must remain loaded (saw ${_testUtils.getLoadedTelemetryCount()})`, + ); + + // The rendered Log must show all three telemetry entries (the bug dropped + // the intermediate one, leaving 2). + const chat = env.document.getElementById('chat'); + const html = chat ? chat.innerHTML : ''; + assert.equal( + countOccurrences(html, 'Broadcasted telemetry'), + 3, + `the Log must show all three telemetry entries (saw ${countOccurrences(html, 'Broadcasted telemetry')})`, + ); + } finally { + globalThis.fetch = originalFetch; + env.cleanup(); + } +}); + +/** Build a resolved fetch-style response around a JSON body. */ +function jsonResponse(body) { + return Promise.resolve({ ok: true, status: 200, json: () => Promise.resolve(body) }); +} diff --git a/web/public/assets/js/app/chat-log-tabs.js b/web/public/assets/js/app/chat-log-tabs.js index 4ea9f8fd..d79ca8ba 100644 --- a/web/public/assets/js/app/chat-log-tabs.js +++ b/web/public/assets/js/app/chat-log-tabs.js @@ -181,11 +181,23 @@ export function buildChatTabModel({ // last_heard is already claimed, so each heard yields exactly one Log entry // and message bodies are recorded as node-info updates, never echoed (LV7). const claimedHeards = new Set(); - const claimKey = (nodeId, nodeNum, ts) => `${nodeId ?? ''}:${nodeNum ?? ''}:${ts ?? ''}`; - // Every call site validates ``ts`` (>= cutoff) before claiming, so no null guard. - const claimHeard = (ts, nodeId, nodeNum) => { - claimedHeards.add(claimKey(nodeId, nodeNum, ts)); + // A heard is claimed by its canonical node id + ts. The canonical ``!%08x`` id + // is derived from ``node_num`` when a record carries only the number + // ({@link normaliseNodeId}), so the id alone identifies a node across every + // event shape. The prior key folded in ``node_num`` and required BOTH to match, + // so a specific event carrying only ``node_id`` (``node_num`` is int|nil per + // CONTRACTS, and is commonly nil — notably for MeshCore) failed to claim the + // node record's heard and leaked a redundant "Updated node info (advert)" line + // (bugfix A2). + const claimKey = (nodeId, ts) => `${nodeId}:${ts}`; + // Every call site validates ``ts`` (>= cutoff) before claiming, so no ts guard. + // An id-less event (no node_id and no derivable node_num) claims nothing — it + // cannot identify a node, so it must not suppress another node's advert. + const claimHeard = (ts, nodeId) => { + if (nodeId) claimedHeards.add(claimKey(nodeId, ts)); }; + // True when a more-specific event already claimed this node's heard at ``ts``. + const heardClaimed = (nodeId, ts) => Boolean(nodeId) && claimedHeards.has(claimKey(nodeId, ts)); // A node's last_heard yields an "updated node info (advert)" entry unless a // more-specific event claims that timestamp. Collected up front and resolved // after every specific event is processed, so source ordering is irrelevant. @@ -220,7 +232,7 @@ export function buildChatTabModel({ const nodeId = normaliseNodeId(snapshot); const nodeNum = normaliseNodeNum(snapshot); logEntries.push({ ts, type: CHAT_LOG_ENTRY_TYPES.TELEMETRY, telemetry: snapshot, nodeId, nodeNum }); - claimHeard(ts, nodeId, nodeNum); + claimHeard(ts, nodeId); } } @@ -236,7 +248,7 @@ export function buildChatTabModel({ const nodeId = normaliseNodeId(snapshot); const nodeNum = normaliseNodeNum(snapshot); logEntries.push({ ts, type: CHAT_LOG_ENTRY_TYPES.POSITION, position: snapshot, nodeId, nodeNum }); - claimHeard(ts, nodeId, nodeNum); + claimHeard(ts, nodeId); } } @@ -250,7 +262,7 @@ export function buildChatTabModel({ const nodeNum = normaliseNodeNum(snapshot); const neighborId = normaliseNeighborId(snapshot); logEntries.push({ ts, type: CHAT_LOG_ENTRY_TYPES.NEIGHBOR, neighbor: snapshot, nodeId, nodeNum, neighborId }); - claimHeard(ts, nodeId, nodeNum); + claimHeard(ts, nodeId); } } @@ -280,7 +292,7 @@ export function buildChatTabModel({ nodeId: firstHop.id ?? null, nodeNum: firstHop.num ?? null }); - claimHeard(ts, firstHop.id ?? null, firstHop.num ?? null); + claimHeard(ts, firstHop.id ?? null); } const encryptedLogEntries = []; @@ -297,6 +309,11 @@ export function buildChatTabModel({ encryptedLogKeys.add(key); encryptedLogEntries.push({ ts, type: CHAT_LOG_ENTRY_TYPES.MESSAGE_ENCRYPTED, message }); } + // The "🔒 encrypted message" line is this sender's Log representation for the + // heard (the sender id is known even though the body is encrypted), so claim + // it — a decrypted message does the same — suppressing the redundant + // "(advert)" line for the same node + ts (LV7). + claimHeard(ts, normaliseNodeId(message.from_id ?? message.fromId)); continue; } @@ -370,7 +387,7 @@ export function buildChatTabModel({ nodeNum: fromNum }); } - claimHeard(ts, fromId, fromNum); + claimHeard(ts, fromId); } } @@ -385,6 +402,9 @@ export function buildChatTabModel({ } encryptedLogKeys.add(key); encryptedLogEntries.push({ ts, type: CHAT_LOG_ENTRY_TYPES.MESSAGE_ENCRYPTED, message }); + // Claim the sender's heard so the redundant "(advert)" line is suppressed, + // matching the main message loop (LV7). + claimHeard(ts, normaliseNodeId(message.from_id ?? message.fromId)); } if (encryptedLogEntries.length > 0) { @@ -396,7 +416,7 @@ export function buildChatTabModel({ // decrypted message), realising the "unless another specific type was already // emitted" rule. for (const candidate of advertCandidates) { - if (claimedHeards.has(claimKey(candidate.nodeId, candidate.nodeNum, candidate.ts))) { + if (heardClaimed(candidate.nodeId, candidate.ts)) { continue; } logEntries.push({ diff --git a/web/public/assets/js/app/chat-tabs.js b/web/public/assets/js/app/chat-tabs.js index 136805a6..9c8ab9b0 100644 --- a/web/public/assets/js/app/chat-tabs.js +++ b/web/public/assets/js/app/chat-tabs.js @@ -14,6 +14,64 @@ * limitations under the License. */ +/** + * Slack (px) within which the active panel counts as scrolled to the bottom. + * A reader inside this band is treated as "pinned" and kept pinned across a + * passive re-render (tail-follow); anyone scrolled further up keeps their exact + * position. Small enough to ignore sub-pixel rounding, large enough to survive a + * one-line layout jitter. + * @type {number} + */ +const SCROLL_PIN_TOLERANCE_PX = 4; + +/** + * Capture the active panel's vertical scroll state before the subtree rebuild, + * so a passive re-render can restore it instead of yanking the reader to the + * bottom on every live update (bugfix B). ``renderChatTabs`` replaces the whole + * panel subtree, so the post-render panel is a fresh element with ``scrollTop`` + * 0; without this capture the reader's position is lost on every refresh. + * + * @param {HTMLElement} container Chat container holding the *previous* render. + * @returns {?{ top: number, pinned: boolean }} The active panel's scroll offset + * and whether it was at the bottom, or ``null`` when there is no prior panel + * (initial render) — in which case the caller pins to the bottom. + */ +function capturePreviousActivePanelScroll(container) { + const panelWrapper = container && container.children && container.children[1]; + const panels = panelWrapper && panelWrapper.children; + if (!panels) { + return null; + } + for (const panel of panels) { + if (!panel || panel.hidden !== false) continue; + const distanceFromBottom = panel.scrollHeight - panel.scrollTop - panel.clientHeight; + return { top: panel.scrollTop, pinned: distanceFromBottom <= SCROLL_PIN_TOLERANCE_PX }; + } + return null; +} + +/** + * Apply the captured vertical scroll to the freshly-rendered active panel. + * A reader that was pinned to the bottom (or an initial render with no prior + * panel, ``previous == null``) is scrolled to the new bottom so freshly-arrived + * entries stay visible (tail-follow); otherwise the reader's exact offset is + * restored (bugfix B). + * + * @param {?HTMLElement} panel The active panel after the rebuild. + * @param {?{ top: number, pinned: boolean }} previous Captured scroll state. + * @returns {void} + */ +function applyActivePanelScroll(panel, previous) { + if (!panel) { + return; + } + if (!previous || previous.pinned) { + panel.scrollTop = panel.scrollHeight; + } else { + panel.scrollTop = previous.top; + } +} + /** * Render an accessible tab interface within ``container``. * @@ -121,6 +179,9 @@ export function renderChatTabs({ typeof previousTabList.scrollLeft === 'number' ? previousTabList.scrollLeft : 0; + // Capture the active panel's VERTICAL scroll before the rebuild so a passive + // refresh restores it rather than snapping the reader to the bottom (bugfix B). + const previousActivePanelScroll = capturePreviousActivePanelScroll(container); const activeCandidateOrder = [existingActive, previousActiveTabId, defaultActiveTabId]; let activeTabId = null; @@ -259,14 +320,17 @@ export function renderChatTabs({ matched = true; container.dataset.activeTab = newId; tabSelect.value = newId; - if (typeof entry.panel.scrollHeight === 'number' && typeof entry.panel.scrollTop === 'number') { - entry.panel.scrollTop = entry.panel.scrollHeight; - } - // Scroll the active tab button into view within the overflow tab list, - // but only on an explicit user tab switch (item 5): a passive re-render - // keeps the user's current horizontal scroll instead of yanking it. - if (scrollActiveIntoView && typeof entry.button.scrollIntoView === 'function') { - entry.button.scrollIntoView({ block: 'nearest', inline: 'nearest' }); + // An explicit tab switch (click / dropdown) jumps to the newest entry and + // scrolls the chosen tab into view. A passive re-render does NEITHER: the + // reader's vertical scroll is restored by the caller below, and the + // horizontal tab scroll is left untouched (bugfix B / LD-A2). + if (scrollActiveIntoView) { + if (typeof entry.panel.scrollHeight === 'number' && typeof entry.panel.scrollTop === 'number') { + entry.panel.scrollTop = entry.panel.scrollHeight; + } + if (typeof entry.button.scrollIntoView === 'function') { + entry.button.scrollIntoView({ block: 'nearest', inline: 'nearest' }); + } } } else { entry.button.classList.remove('is-active'); @@ -280,6 +344,15 @@ export function renderChatTabs({ setActiveTab(activeTabId); + // Restore the active panel's vertical scroll captured before the rebuild: a + // reader pinned to the bottom stays pinned (tail-follow), anyone scrolled up + // keeps their place, and an initial render (no prior panel) pins to the + // bottom so the newest entries show (bugfix B). + // ``activeTabId`` is always one of ``tabElements`` (chosen from them above), so + // the entry resolves; ``applyActivePanelScroll`` still guards a missing panel. + const restoredActiveEntry = tabElements.find(entry => entry.id === activeTabId); + applyActivePanelScroll(restoredActiveEntry.panel, previousActivePanelScroll); + // Restore the horizontal scroll captured before the rebuild so a live // refresh does not reset the channel-tab list to the first tab (item 5). // Applied after setActiveTab, which no longer force-scrolls on a passive @@ -323,4 +396,9 @@ function createFragment(document) { }; } -export const __test__ = { createFragment }; +export const __test__ = { + createFragment, + SCROLL_PIN_TOLERANCE_PX, + capturePreviousActivePanelScroll, + applyActivePanelScroll +}; diff --git a/web/public/assets/js/app/main.js b/web/public/assets/js/app/main.js index 9cf8dfea..acaee76d 100644 --- a/web/public/assets/js/app/main.js +++ b/web/public/assets/js/app/main.js @@ -98,7 +98,6 @@ import { buildMessageIndex } from './message-replies.js'; import { renderChatEntryContent } from './chat-entry-renderer.js'; import { SNAPSHOT_WINDOW, - aggregateNeighborSnapshots, aggregateNodeSnapshots, aggregatePositionSnapshots, aggregateTelemetrySnapshots, @@ -129,7 +128,6 @@ import { // remain locally callable inside ``initializeApp`` because module-level // bindings are visible in every nested scope. import { - cssEscape, fmtCoords, fmtHw, formatDate, @@ -241,7 +239,6 @@ export function initializeApp(config) { ? String(document.body.dataset.privateMode).toLowerCase() === 'true' : false; const isDashboardView = bodyClassList ? bodyClassList.contains('view-dashboard') : false; - const isChatView = bodyClassList ? bodyClassList.contains('view-chat') : false; const isMapView = bodyClassList ? bodyClassList.contains('view-map') : false; const mapZoomOverride = Number.isFinite(config.mapZoom) ? Number(config.mapZoom) : null; @@ -658,30 +655,6 @@ export function initializeApp(config) { } } - /** - * Scroll the active chat tab panel to its most recent entry when the - * dedicated chat view is displayed. - * - * @returns {void} - */ - function scrollActiveChatPanelToBottom() { - if (!chatEl || !isChatView) { - return; - } - const activeTabId = chatEl.dataset?.activeTab; - if (!activeTabId) { - return; - } - const escapedId = cssEscape(activeTabId); - if (!escapedId) { - return; - } - const panel = chatEl.querySelector(`#chat-panel-${escapedId}`); - if (panel && typeof panel.scrollHeight === 'number' && typeof panel.scrollTop === 'number') { - panel.scrollTop = panel.scrollHeight; - } - } - /** * Sort a list of nodes using the active table sorter configuration. * @@ -3341,7 +3314,10 @@ export function initializeApp(config) { previousActiveTabId: previousActive, defaultActiveTabId: defaultActive }); - scrollActiveChatPanelToBottom(); + // renderChatTabs now owns chat-panel scroll: it pins to the bottom on the + // initial render and tail-follows a bottom-pinned reader, but preserves the + // vertical position on a passive live refresh (bugfix B). No extra + // force-scroll here — that was what reset the reader to the bottom every tick. } /** @@ -3525,7 +3501,6 @@ export function initializeApp(config) { function rebuildNodeDerivedState() { const aggregatedNodes = aggregateNodeSnapshots(allNodes); const aggregatedPositions = aggregatePositionSnapshots(allPositionEntries); - const aggregatedNeighbors = aggregateNeighborSnapshots(allNeighbors); const aggregatedTelemetry = aggregateTelemetrySnapshots(allTelemetryEntries); // Enrich merged node records with display name, position, distance, and // telemetry before any rendering or filtering takes place. @@ -3538,9 +3513,16 @@ export function initializeApp(config) { // Rebuild lookup maps so marker updates and message hydration always resolve // to the latest node objects. rebuildNodeIndex(allNodes); - allTelemetryEntries = aggregatedTelemetry; - allPositionEntries = aggregatedPositions; - allNeighbors = aggregatedNeighbors; + // The per-packet accumulators (allTelemetryEntries / allPositionEntries / + // allNeighbors) are deliberately left RAW — the aggregated forms above are + // locals used only to enrich the node records. Writing an aggregate back into + // an accumulator would feed it into the next refresh's merge + re-aggregation, + // which is lossy: aggregateSnapshots clones with ``{...snapshot}`` (dropping + // the non-enumerable ``snapshots`` history) and merges oldest-last (pinning to + // the stalest reading), collapsing each node's history to {stale-first, newest} + // so a telemetry/position Log entry flashes in and vanishes on the next tick. + // Keeping the accumulators raw gives every packet a stable, id-keyed Log entry + // (bugfix A1). } /** Floor (unix s) below which backfilled positions/telemetry are dropped (FC3: 7 d). */ @@ -3603,7 +3585,10 @@ export function initializeApp(config) { longBackfillFloor(), ); }, - refine: () => { allNeighbors = aggregateNeighborSnapshots(allNeighbors); }, + // Neighbors stay RAW (like traces) so the Log keeps every per-pair snapshot + // and the map / overlay consumers dedupe internally; re-aggregating here + // would erode history exactly as the refresh path used to (bugfix A1). + refine: () => {}, }, { name: 'traces', @@ -4798,11 +4783,12 @@ export function initializeApp(config) { ? trimToWindow(mergeById(allMessages, incomingMessages, 'id'), messageWindowFloor) : incomingMessages; - // Collapse per-source snapshots and enrich the node collection from the - // merged sources. Shared with the background backfill so a streamed page - // re-derives identically (issue #832); this also re-aggregates and stores - // allPositionEntries/allTelemetryEntries/allNeighbors in their collapsed - // form (allTraces is not node-derived and keeps the merged value above). + // Aggregate per-source snapshots into locals and enrich the node collection + // from the merged sources. Shared with the background backfill so a streamed + // page re-derives identically (issue #832). The per-packet accumulators + // (allPositionEntries/allTelemetryEntries/allNeighbors) are left RAW so the + // Log keeps a stable entry per packet — re-storing the aggregated form would + // erode history on the next tick (bugfix A1). rebuildNodeDerivedState(); // Hydrate messages with node metadata in parallel; the node index has just // been rebuilt (inside rebuildNodeDerivedState) so lookups find the freshly