diff --git a/src/renderer/components/ChatPanel.test.tsx b/src/renderer/components/ChatPanel.test.tsx index b68cbe6f9..70d3e7a45 100644 --- a/src/renderer/components/ChatPanel.test.tsx +++ b/src/renderer/components/ChatPanel.test.tsx @@ -5,7 +5,13 @@ import { axe } from 'vitest-axe'; import { hydrateAxeThemeColors } from '../lib/a11yTestHelpers'; import * as chatNotifications from '../lib/chatNotifications'; -import { draftsStorageKey, lastReadStorageKey, saveDraft } from '../lib/chatPanelProtocolStorage'; +import { + draftsStorageKey, + lastReadStorageKey, + loadActiveChannelInitial, + saveActiveChannel, + saveDraft, +} from '../lib/chatPanelProtocolStorage'; import { getDistFromChatBottom, VIRTUALIZER_SCROLL_END_THRESHOLD } from '../lib/chatScrollUtils'; import i18n from '../lib/i18n'; import { ensureLocaleLoaded } from '../lib/localeResources'; @@ -3705,6 +3711,408 @@ describe('ChatPanel — draft restored on initial mount', () => { }); }); +describe('ChatPanel — channel selection restored across reconnect', () => { + it('restores the previously selected channel for this node on mount', async () => { + localStorage.clear(); + saveActiveChannel('meshtastic', 1, 1); // baseProps.myNodeNum is 1; select channel index 1 + saveDraft('meshtastic', 'ch:1', 'admin draft'); // distinguishes which channel is active + + render( + + + , + ); + + const textarea = await waitForComposer(); + expect(textarea).toHaveValue('admin draft'); + + localStorage.setItem(draftsStorageKey('meshtastic'), '{}'); + }); + + it('falls back to the default channel when the persisted selection belongs to a different node', async () => { + localStorage.clear(); + saveActiveChannel('meshtastic', 999, 1); // a different node's saved selection + saveDraft('meshtastic', 'ch:0', 'general draft'); + + render( + + + , + ); + + const textarea = await waitForComposer(); + expect(textarea).toHaveValue('general draft'); + + localStorage.setItem(draftsStorageKey('meshtastic'), '{}'); + }); + + it('restores a saved channel once myNodeNum becomes known after mount, without clobbering it', async () => { + // ChatPanel mounts once per protocol tab and can do so before the radio finishes + // connecting (myNodeNum still 0) — the restore must re-run once myNodeNum arrives, + // and must not immediately overwrite the just-restored value with the pre-restore + // default (regression: both the restore and the save effect fire on the same + // myNodeNum-changing commit). + localStorage.clear(); + saveActiveChannel('meshtastic', 1, 1); // saved from a prior session for node 1 + saveDraft('meshtastic', 'ch:1', 'admin draft'); + + const channels = [ + { index: 0, name: 'General' }, + { index: 1, name: 'Admin' }, + ]; + + const { rerender } = render( + + + , + ); + await waitForComposer(); + + rerender( + + + , + ); + + const textarea = await waitForComposer(); + await waitFor(() => { + expect(textarea).toHaveValue('admin draft'); + }); + // The saved value must survive the restore — not get clobbered back to the default. + expect(loadActiveChannelInitial('meshtastic', 1)).toBe(1); + + localStorage.setItem(draftsStorageKey('meshtastic'), '{}'); + }); + + it("does not leak the previous node's channel into a different node's saved key on a live switch", async () => { + // Regression (CodeRabbit, PR #858): switching from node A (has a saved + // selection) to node B (no saved value yet) while ChatPanel stays mounted + // must not persist A's channel under B's key — even though `channels` can + // transiently still show A's stale, carried-forward list right after the + // switch (see useMeshtasticRuntime's lastKnownChannelsRef). + localStorage.clear(); + saveActiveChannel('meshtastic', 1, 1); // node 1 (A) previously selected Admin (index 1) + + const channels = [ + { index: 0, name: 'General' }, + { index: 1, name: 'Admin' }, + ]; + + const { rerender } = render( + + + , + ); + await waitForComposer(); + + // Switch to node 2 (B) while the panel stays mounted; channels prop still + // shows A's list, as it would transiently during a live node switch. + rerender( + + + , + ); + await waitForComposer(); + + await waitFor(() => { + expect(loadActiveChannelInitial('meshtastic', 2)).not.toBeNull(); + }); + expect(loadActiveChannelInitial('meshtastic', 2)).not.toBe(1); // A's index must not leak + expect(loadActiveChannelInitial('meshtastic', 1)).toBe(1); // A's own saved value untouched + }); + + it("keeps node B's saved channel pending (not overwritten) while the list still belongs to node A, then restores it once B's real channels arrive", async () => { + // Regression (CodeRabbit, PR #858 second pass): a saved value for the new + // node/scope must not be treated as "nothing saved" just because the + // current (stale, carried-forward) channels list doesn't contain it yet — + // that previously forced a default selection and then persisted it, + // clobbering the real saved value the moment it became visible. + localStorage.clear(); + saveActiveChannel('meshtastic', 2, 2); // node B (2) previously selected channel index 2 + saveDraft('meshtastic', 'ch:2', 'node b draft'); + + const staleChannelsFromNodeA = [ + { index: 0, name: 'General' }, + { index: 1, name: 'Admin' }, + ]; + + const { rerender } = render( + + + , + ); + await waitForComposer(); + + // Switch to node B; channels prop still shows A's stale list (no index 2). + rerender( + + + , + ); + await waitForComposer(); + + // While pending: must NOT have clobbered node B's saved value with a default. + expect(loadActiveChannelInitial('meshtastic', 2)).toBe(2); + + // Node B's real channel list arrives. + const realChannelsFromNodeB = [ + { index: 0, name: 'General' }, + { index: 2, name: 'Ops' }, + ]; + rerender( + + + , + ); + + const textarea = await waitForComposer(); + await waitFor(() => { + expect(textarea).toHaveValue('node b draft'); + }); + expect(loadActiveChannelInitial('meshtastic', 2)).toBe(2); + }); + + it('stops waiting for a saved channel that no longer exists once the list stabilizes, so a later selection still saves', async () => { + // Self-caught regression: without a bound on "pending", a saved channel + // that's genuinely gone from this node's config (removed since the last + // session) would leave restoration pending forever — and since saving is + // suppressed while pending, that would silently disable persisting *any* + // future selection for this node, not just fail to restore the old one. + localStorage.clear(); + saveActiveChannel('meshtastic', 3, 99); // node 3 previously had channel 99 — no longer present + + const channelsAttempt1 = [ + { index: 0, name: 'General' }, + { index: 1, name: 'Admin' }, + ]; + const { rerender } = render( + + + , + ); + await waitForComposer(); + + // Channel list re-renders with the same content (new array reference, same + // indices) — simulates the list having settled without ever containing 99. + const channelsAttempt2 = [ + { index: 0, name: 'General' }, + { index: 1, name: 'Admin' }, + ]; + rerender( + + + , + ); + await waitForComposer(); + + // A later, deliberate channel pick must still get persisted — proves + // saving isn't stuck suppressed forever. + const user = userEvent.setup(); + const adminButton = screen + .getAllByRole('button') + .find((b) => /Admin/i.test(b.textContent ?? '')); + expect(adminButton).toBeTruthy(); + if (adminButton) await user.click(adminButton); + + await waitFor(() => { + expect(loadActiveChannelInitial('meshtastic', 3)).toBe(1); // Admin's index + }); + }); + + it('lets a manual channel click win over a still-pending restore, instead of being silently overwritten once it resolves', async () => { + // Self-caught regression: if a restore is still pending (waiting for a + // saved value to show up in `channels`) when the user manually picks a + // *different* channel, the restore effect must not later fire and + // silently override that manual pick once the saved value's channel + // finally appears in the list. + localStorage.clear(); + saveActiveChannel('meshtastic', 4, 2); // node 4 previously selected channel index 2 + + const staleChannels = [ + { index: 0, name: 'General' }, + { index: 1, name: 'Admin' }, + ]; + // Mount on a different node first, then switch to node 4 while `channels` + // still shows the stale list — the lazy initializer only runs at true + // first mount, so this is what actually exercises the pending-restore + // effect (mounting directly at myNodeNum=4 would resolve immediately via + // the initializer instead, never engaging "pending" at all). + const { rerender } = render( + + + , + ); + await waitForComposer(); + rerender( + + + , + ); + await waitForComposer(); + // Still pending: saved value (2) isn't in the current list yet. + expect(loadActiveChannelInitial('meshtastic', 4)).toBe(2); + + // User manually picks Admin (1) while restoration is still pending. + const user = userEvent.setup(); + const adminButton = screen + .getAllByRole('button') + .find((b) => /Admin/i.test(b.textContent ?? '')); + expect(adminButton).toBeTruthy(); + if (adminButton) await user.click(adminButton); + await waitFor(() => { + expect(loadActiveChannelInitial('meshtastic', 4)).toBe(1); + }); + + // The saved value's channel (2) now shows up in the list — must NOT + // silently override the user's manual pick. + const realChannels = [ + { index: 0, name: 'General' }, + { index: 1, name: 'Admin' }, + { index: 2, name: 'Ops' }, + ]; + rerender( + + + , + ); + await waitForComposer(); + + expect(loadActiveChannelInitial('meshtastic', 4)).toBe(1); + }); + + it('retries the restore once channels arrive, even when myNodeNum was already known at the very first mount', async () => { + // Self-caught regression: myNodeNum and channels come from separate + // packets, so channels can easily still be empty on the very first + // render even though the node is already known. The restore-state ref's + // initializer must not unconditionally mark that "resolved" — it has to + // mirror whatever the lazy `channel` initializer actually found, or a + // saved value that arrives moments later would never get retried. + localStorage.clear(); + saveActiveChannel('meshtastic', 5, 3); + saveDraft('meshtastic', 'ch:3', 'ops draft'); + + const { rerender } = render( + + + , + ); + await waitForComposer(); + + const realChannels = [ + { index: 0, name: 'General' }, + { index: 3, name: 'Ops' }, + ]; + rerender( + + + , + ); + + const textarea = await waitForComposer(); + await waitFor(() => { + expect(textarea).toHaveValue('ops draft'); + }); + expect(loadActiveChannelInitial('meshtastic', 5)).toBe(3); + }); + + it("does not leak a different node's leftover channel selection when giving up on a saved channel that no longer exists", async () => { + // Self-caught regression: giving up on a saved-but-never-found channel + // for a *different* scope must still reset away from the previous + // scope's leftover selection — even when that leftover index happens to + // also be "valid" in the new (now-stable) list, which is exactly what + // the pre-existing clamp effect can't catch on its own. + localStorage.clear(); + saveActiveChannel('meshtastic', 1, 1); // node 1 (A): selected Admin (index 1) + saveActiveChannel('meshtastic', 2, 99); // node 2 (B): saved channel 99 — no longer exists + + const channelsWithAdmin = [ + { index: 0, name: 'General' }, + { index: 1, name: 'Admin' }, + ]; + const { rerender } = render( + + + , + ); + await waitForComposer(); + + // Switch to node B; its real list happens to also contain index 1 + // (Admin) — A's leftover selection — but never contains 99. + rerender( + + + , + ); + await waitForComposer(); + expect(loadActiveChannelInitial('meshtastic', 2)).toBe(99); // still pending, untouched + + // List "stabilizes" (same content, new array reference) without ever + // containing 99 — give up. + rerender( + + + , + ); + await waitForComposer(); + + await waitFor(() => { + expect(loadActiveChannelInitial('meshtastic', 2)).not.toBe(1); // must not leak A's index + }); + expect(loadActiveChannelInitial('meshtastic', 2)).toBe(0); // reset to default (General) + expect(loadActiveChannelInitial('meshtastic', 1)).toBe(1); // A's own saved value untouched + }); +}); + describe('ChatPanel — notification sound on new messages', () => { const playMock = vi.mocked(chatNotifications.playMessageNotification); diff --git a/src/renderer/components/ChatPanel.tsx b/src/renderer/components/ChatPanel.tsx index b1d839846..13b4c7a43 100644 --- a/src/renderer/components/ChatPanel.tsx +++ b/src/renderer/components/ChatPanel.tsx @@ -87,6 +87,7 @@ import { playMessageNotification } from '../lib/chatNotifications'; import { dismissedDmTabsStorageKey, lastReadStorageKey, + loadActiveChannelInitial, loadActiveDmInitial, loadMutedViews, loadOpenDmTabsInitial, @@ -94,6 +95,7 @@ import { loadStarred, notifyPersistedLastReadChanged, openDmTabsStorageKey, + saveActiveChannel, saveActiveDm, saveMutedViews, saveStarred, @@ -655,12 +657,215 @@ function ChatPanel({ }, []); useImperativeHandle(scrollToTopRef, () => scrollToTop, [scrollToTop]); - const [channel, setChannel] = useState(() => (channels.length > 0 ? channels[0].index : 0)); + const defaultChannelIndex = channels.length > 0 ? channels[0].index : 0; + const [channel, setChannel] = useState(() => { + const persisted = myNodeNum > 0 ? loadActiveChannelInitial(protocol, myNodeNum) : null; + if (persisted != null && channels.some((c) => c.index === persisted)) return persisted; + return defaultChannelIndex; + }); useEffect(() => { if (channels.length > 0 && !channels.some((c) => c.index === channel)) { - setChannel(channels[0].index); + setChannel(defaultChannelIndex); } - }, [channels, channel]); + }, [channels, channel, defaultChannelIndex]); + /** + * ChatPanel mounts once per protocol tab and often before the radio finishes + * connecting, so `myNodeNum` can still be 0 (no restore attempted) at the lazy + * initializer above. Re-attempt the restore once a real node number is known — + * covers both "connected after mount" and "switched to a different node while + * this panel stayed mounted". Scoped by protocol + node (not node alone): a + * protocol switch remounts ChatPanel (App.tsx keys it on protocol) so this is + * belt-and-suspenders, but costs nothing. + */ + const channelRestoreScopeKey = myNodeNum > 0 ? `${protocol}:${myNodeNum}` : null; + interface ChannelRestoreState { + scope: string | null; + resolved: boolean; + indexSignature: string | null; + /** + * Whether this scope was ever entered from a genuinely *different* prior + * scope — captured once when a scope is first seen and carried forward + * unchanged through every later re-check of that same scope (recomputing + * "did the scope just change" fresh on each pending retry reads false + * once we've already been pending on it for a tick, which is exactly + * when the leak-prevention reset below is needed most: the moment + * restoration gives up on a stale value, `channel` can still be the + * *previous* scope's leftover selection). + */ + arrivedFromDifferentScope: boolean; + } + /** + * `resolved: false` means restoration for `scope` hasn't been settled yet — + * either a saved value exists for it but the current `channels` list hasn't + * confirmed it (e.g. still showing a *previous* node's stale, carried-forward + * list — see useMeshtasticRuntime's lastKnownChannelsRef), or `myNodeNum` is + * still 0. Saving is suppressed the whole time a scope is unresolved: a saved + * value not yet found in a stale list is NOT the same as "no saved value" — + * treating them the same let a real save get clobbered by the default the + * moment a different node's carried-forward list didn't happen to contain it. + * `indexSignature` is the set of indices `channels` had on the last pending + * check for this scope, used only to detect the list has *stopped* changing + * (see below) — never to decide whether the saved value matches. + * + * The initial value mirrors the `channel` lazy initializer above rather than + * assuming "resolved" outright: if `myNodeNum` is already known at mount but + * `channels` hasn't arrived yet (a normal race — they come from separate + * packets), a saved value can't be found on that first render either, and + * marking it resolved unconditionally would permanently skip ever retrying + * once the real list arrives with the match. + * + * Computed via an "initialized" guard rather than directly as `useRef`'s + * argument — that argument is evaluated on every render even though only + * the first one is ever used, and ChatPanel re-renders often (every + * message, every scroll update), which would repeat the localStorage read + * below on every one of those renders for no reason. + */ + const channelRestoreInitializedRef = useRef(false); + const channelRestoreRef = useRef({ + scope: null, + resolved: true, + indexSignature: null, + arrivedFromDifferentScope: false, + }); + if (!channelRestoreInitializedRef.current) { + channelRestoreInitializedRef.current = true; + if (channelRestoreScopeKey != null) { + const persisted = loadActiveChannelInitial(protocol, myNodeNum); + channelRestoreRef.current = + persisted == null || channels.some((c) => c.index === persisted) + ? { + scope: channelRestoreScopeKey, + resolved: true, + indexSignature: null, + arrivedFromDifferentScope: false, + } + : // `indexSignature: null` (not the real, computed signature) — + // otherwise the very first restore-effect run right after mount + // would compare against this same unchanged snapshot, see a + // "match" on indexSignature alone, and give up immediately + // before `channels` ever gets a chance to actually update. + // Stability can only be concluded by comparing two *effect* + // observations, never the initializer's own snapshot against + // itself. + { + scope: channelRestoreScopeKey, + resolved: false, + indexSignature: null, + arrivedFromDifferentScope: false, + }; + } + } + /** True for the one save-effect run right after a restore/reset-triggered setChannel, + * so that run doesn't persist the pre-transition value it hasn't caught up to yet. */ + const skipNextChannelSaveRef = useRef(false); + useEffect(() => { + if (channelRestoreScopeKey == null) return; + const prior = channelRestoreRef.current; + if (prior.scope === channelRestoreScopeKey && prior.resolved) return; // already settled + const arrivedFromDifferentScope = + prior.scope === channelRestoreScopeKey + ? prior.arrivedFromDifferentScope + : prior.scope !== null; + + const persisted = loadActiveChannelInitial(protocol, myNodeNum); + if (persisted != null && channels.some((c) => c.index === persisted)) { + channelRestoreRef.current = { + scope: channelRestoreScopeKey, + resolved: true, + indexSignature: null, + arrivedFromDifferentScope, + }; + if (persisted !== channel) { + skipNextChannelSaveRef.current = true; + setChannel(persisted); + } + return; + } + if (persisted != null) { + // A value IS saved for this scope, but `channels` doesn't contain it. + // Could mean the list hasn't finished arriving yet (stay pending, keep + // saving suppressed, re-check next `channels` change) — OR the channel + // was genuinely removed from this node's config since it was saved, in + // which case the list will stop changing and we must NOT wait forever: + // that would silently disable saving *any* future selection for this + // node for the rest of the session. Give up once the set of available + // indices is identical to the last pending check (content-stable, not + // just a new array reference). + const indexSignature = channels.map((c) => c.index).join(','); + if (prior.scope === channelRestoreScopeKey && prior.indexSignature === indexSignature) { + channelRestoreRef.current = { + scope: channelRestoreScopeKey, + resolved: true, + indexSignature: null, + arrivedFromDifferentScope, + }; + // Fall through to the "nothing to restore" handling below — same + // outcome as if nothing had ever been saved for this scope. + } else { + channelRestoreRef.current = { + scope: channelRestoreScopeKey, + resolved: false, + indexSignature, + arrivedFromDifferentScope, + }; + return; + } + } else { + channelRestoreRef.current = { + scope: channelRestoreScopeKey, + resolved: true, + indexSignature: null, + arrivedFromDifferentScope, + }; + } + // Nothing to restore (never saved, or saved but genuinely gone). If this + // scope was ever arrived at from a *different* scope, `channel` may still + // hold that other scope's index and `channels` may still be showing its + // stale, carried-forward list — don't let that leak into this scope's + // saved preference. Force back to the default explicitly; the + // pre-existing clamp effect above can't catch this because the stale + // index is still "valid" against the stale list. + if (arrivedFromDifferentScope && defaultChannelIndex !== channel) { + skipNextChannelSaveRef.current = true; + setChannel(defaultChannelIndex); + } + }, [protocol, myNodeNum, channelRestoreScopeKey, channels, channel, defaultChannelIndex]); + useEffect(() => { + if (channelRestoreScopeKey == null) return; + const status = channelRestoreRef.current; + if (status.scope !== channelRestoreScopeKey || !status.resolved) return; // still pending + if (skipNextChannelSaveRef.current) { + skipNextChannelSaveRef.current = false; + return; + } + // Only persist a selection the current channel list actually has — never a + // momentarily-invalid index from a channel list that just shrank (the clamp + // effect above will correct `channel` next render; this run simply skips). + if (!channels.some((c) => c.index === channel)) return; + saveActiveChannel(protocol, myNodeNum, channel); + }, [protocol, myNodeNum, channelRestoreScopeKey, channel, channels]); + /** + * Deliberate, user-initiated channel selection. Immediately marks this + * scope's restore as resolved — a race otherwise exists where a restore is + * still pending (waiting for a saved value to show up in `channels`) when + * the user manually picks a *different* channel; if the saved value later + * matches, the pending restore would fire and silently overwrite the user's + * manual pick. A deliberate selection always wins and unblocks saving. + */ + const selectChannel = useCallback( + (index: number) => { + if (channelRestoreScopeKey != null) { + channelRestoreRef.current = { + scope: channelRestoreScopeKey, + resolved: true, + indexSignature: null, + arrivedFromDifferentScope: false, + }; + } + setChannel(index); + }, + [channelRestoreScopeKey], + ); const [chatActionError, setChatActionError] = useState<{ message: string; viewKey: string; @@ -2015,7 +2220,7 @@ function ChatPanel({ key={`ch-${ch.index}-${chIdx}-${ch.name}`} aria-label={`${ch.name}${channelUnreadSuffix}`} onClick={() => { - setChannel(ch.index); + selectChannel(ch.index); setViewMode('channels'); }} className={`relative shrink-0 rounded-full px-3 py-1 text-xs font-medium transition-colors ${ @@ -2491,7 +2696,7 @@ function ChatPanel({ if (type === 'dm' && raw) { openDmTo(Number(raw)); } else { - if (raw !== undefined) setChannel(Number(raw)); + if (raw !== undefined) selectChannel(Number(raw)); setViewMode('channels'); } }} diff --git a/src/renderer/lib/chatPanelProtocolStorage.test.ts b/src/renderer/lib/chatPanelProtocolStorage.test.ts index e327075c7..291cce04a 100644 --- a/src/renderer/lib/chatPanelProtocolStorage.test.ts +++ b/src/renderer/lib/chatPanelProtocolStorage.test.ts @@ -1,6 +1,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { + activeChannelStorageKey, activeDmStorageKey, clearDraft, clearFloodScopeOverride, @@ -11,6 +12,7 @@ import { floodScopeOverridesStorageKey, getSanitizedMeshtasticChatLastRead, lastReadStorageKey, + loadActiveChannelInitial, loadActiveDmInitial, loadDraftsInitial, loadFloodScopeOverridesInitial, @@ -26,6 +28,7 @@ import { sanitizeMeshcoreRoomsLastRead, sanitizeMeshtasticChatLastRead, sanitizeReticulumChatLastRead, + saveActiveChannel, saveActiveDm, saveDraft, saveFloodScopeOverride, @@ -70,6 +73,54 @@ describe('chatPanelProtocolStorage', () => { expect(loadActiveDmInitial('reticulum')).toBeNull(); }); + it('persists and loads last-selected channel per protocol + node number', () => { + expect(loadActiveChannelInitial('meshtastic', 0x12345678)).toBeNull(); + saveActiveChannel('meshtastic', 0x12345678, 3); + expect(localStorage.getItem(activeChannelStorageKey('meshtastic', 0x12345678))).toBe('3'); + expect(loadActiveChannelInitial('meshtastic', 0x12345678)).toBe(3); + + // Different node number (e.g. connected to a different physical device that + // happens to reuse the same internal identity slot) must not see it. + expect(loadActiveChannelInitial('meshtastic', 0x87654321)).toBeNull(); + // Different protocol must not see it either. + expect(loadActiveChannelInitial('meshcore', 0x12345678)).toBeNull(); + }); + + it('ignores invalid inputs for active-channel persistence', () => { + saveActiveChannel('meshtastic', 0, 3); // no node number yet — no-op + expect(loadActiveChannelInitial('meshtastic', 0)).toBeNull(); + + localStorage.setItem(activeChannelStorageKey('meshtastic', 5), 'not-a-number'); + expect(loadActiveChannelInitial('meshtastic', 5)).toBeNull(); + + saveActiveChannel('meshtastic', 5, NaN); + expect(loadActiveChannelInitial('meshtastic', 5)).toBeNull(); + saveActiveChannel('meshtastic', 5, 1.5); + expect(loadActiveChannelInitial('meshtastic', 5)).toBeNull(); + saveActiveChannel('meshtastic', 5, -2); // below the -1 sentinel floor + expect(loadActiveChannelInitial('meshtastic', 5)).toBeNull(); + }); + + it('rejects malformed node numbers and whitespace-only stored values', () => { + // Fractional/infinite node numbers must not read or write a key at all. + saveActiveChannel('meshtastic', 1.5, 3); + expect(loadActiveChannelInitial('meshtastic', 1.5)).toBeNull(); + saveActiveChannel('meshtastic', Infinity, 3); + expect(loadActiveChannelInitial('meshtastic', Infinity)).toBeNull(); + expect(loadActiveChannelInitial('meshtastic', NaN)).toBeNull(); + + // A whitespace-only stored value must not silently parse as channel 0 + // (Number(' ') === 0 in JS). + localStorage.setItem(activeChannelStorageKey('meshtastic', 9), ' '); + expect(loadActiveChannelInitial('meshtastic', 9)).toBeNull(); + }); + + it('persists and loads the MeshCore primary-channel sentinel (-1)', () => { + saveActiveChannel('meshcore', 7, -1); + expect(localStorage.getItem(activeChannelStorageKey('meshcore', 7))).toBe('-1'); + expect(loadActiveChannelInitial('meshcore', 7)).toBe(-1); + }); + it('migrates legacy lastRead only into meshtastic key', () => { localStorage.setItem('mesh-client:lastRead', JSON.stringify({ 'ch:0': 1 })); const mt = loadPersistedLastReadInitial('meshtastic'); diff --git a/src/renderer/lib/chatPanelProtocolStorage.ts b/src/renderer/lib/chatPanelProtocolStorage.ts index aef1f37d5..b12204f59 100644 --- a/src/renderer/lib/chatPanelProtocolStorage.ts +++ b/src/renderer/lib/chatPanelProtocolStorage.ts @@ -121,6 +121,64 @@ export function saveActiveDm(protocol: MeshProtocol, nodeId: number | null): voi } } +/** + * Scoped by the connected device's own node number (not just protocol) so that + * switching to a genuinely different physical node — which can reuse the same + * internal identity slot as the node just disconnected from — doesn't restore a + * channel selection that belonged to the previous device. A same-node + * disconnect/reconnect keeps the same node number, so the prior selection is + * still found and restored; a different node simply misses and falls back to + * the default channel. + */ +export function activeChannelStorageKey(protocol: MeshProtocol, nodeNum: number): string { + return `mesh-client:activeChannel:${protocol}:${nodeNum}`; +} + +/** + * ChatPanel uses channel index -1 as the "primary" sentinel (MeshCore); every + * other valid index is >= 0. Reject anything else, including other negatives. + */ +function isValidChannelIndex(value: number): boolean { + return Number.isFinite(value) && Number.isInteger(value) && value >= -1; +} + +/** Node numbers are always positive integers; reject fractional/infinite input. */ +function isValidNodeNum(value: number): boolean { + return Number.isSafeInteger(value) && value > 0; +} + +/** Last-selected channel index for this protocol + node (null if missing/invalid). */ +export function loadActiveChannelInitial(protocol: MeshProtocol, nodeNum: number): number | null { + if (!isValidNodeNum(nodeNum)) return null; + try { + const raw = localStorage.getItem(activeChannelStorageKey(protocol, nodeNum)); + if (raw == null || raw.trim() === '') return null; + const parsed = Number(raw); + if (!isValidChannelIndex(parsed)) return null; + return parsed; + } catch (e) { + console.debug( + '[chatPanelProtocolStorage] loadActiveChannelInitial failed ' + errLikeToLogString(e), + ); + return null; + } +} + +/** Persist last-selected channel index for this protocol + node. No-op without a node number. */ +export function saveActiveChannel( + protocol: MeshProtocol, + nodeNum: number, + channelIndex: number, +): void { + if (!isValidNodeNum(nodeNum)) return; + if (!isValidChannelIndex(channelIndex)) return; + try { + localStorage.setItem(activeChannelStorageKey(protocol, nodeNum), String(channelIndex)); + } catch (e) { + console.debug('[chatPanelProtocolStorage] saveActiveChannel failed ' + errLikeToLogString(e)); + } +} + export function draftsStorageKey(protocol: MeshProtocol): string { return `mesh-client:drafts:${protocol}`; } diff --git a/src/renderer/lib/meshtastic/resolveMeshtasticChannels.test.ts b/src/renderer/lib/meshtastic/resolveMeshtasticChannels.test.ts new file mode 100644 index 000000000..3c73afa16 --- /dev/null +++ b/src/renderer/lib/meshtastic/resolveMeshtasticChannels.test.ts @@ -0,0 +1,70 @@ +import { describe, expect, it } from 'vitest'; + +import { resolveMeshtasticChannels } from './resolveMeshtasticChannels'; + +describe('resolveMeshtasticChannels', () => { + const REAL = [ + { index: 0, name: 'General' }, + { index: 3, name: 'Ops' }, + ]; + const PLACEHOLDER = [{ index: 0, name: 'Primary' }]; + + it('returns the device record channels when present', () => { + expect( + resolveMeshtasticChannels({ + meshtasticIdentityId: 'id-1', + deviceRecordChannels: REAL, + hookChannels: PLACEHOLDER, + lastKnownChannels: [], + }), + ).toBe(REAL); + }); + + it('bridges the disconnect→rebind gap with the last known list when identity is null', () => { + expect( + resolveMeshtasticChannels({ + meshtasticIdentityId: null, + deviceRecordChannels: undefined, + hookChannels: PLACEHOLDER, + lastKnownChannels: REAL, + }), + ).toBe(REAL); + }); + + it('falls back to the hook placeholder when identity is null and nothing was ever cached', () => { + expect( + resolveMeshtasticChannels({ + meshtasticIdentityId: null, + deviceRecordChannels: undefined, + hookChannels: PLACEHOLDER, + lastKnownChannels: [], + }), + ).toBe(PLACEHOLDER); + }); + + it('does not use the stale cache once a real identity is bound but its own record has no channels yet', () => { + // A genuinely new/different device connecting: identityId is set, but its + // deviceStore record hasn't received channel data yet. Must show the + // generic placeholder, not another device's cached channel list. + expect( + resolveMeshtasticChannels({ + meshtasticIdentityId: 'id-2', + deviceRecordChannels: [], + hookChannels: PLACEHOLDER, + lastKnownChannels: REAL, + }), + ).toBe(PLACEHOLDER); + }); + + it('prefers the device record over the cache even mid-gap once it repopulates', () => { + const updated = [{ index: 5, name: 'New' }]; + expect( + resolveMeshtasticChannels({ + meshtasticIdentityId: null, + deviceRecordChannels: updated, + hookChannels: PLACEHOLDER, + lastKnownChannels: REAL, + }), + ).toBe(updated); + }); +}); diff --git a/src/renderer/lib/meshtastic/resolveMeshtasticChannels.ts b/src/renderer/lib/meshtastic/resolveMeshtasticChannels.ts new file mode 100644 index 000000000..f22086566 --- /dev/null +++ b/src/renderer/lib/meshtastic/resolveMeshtasticChannels.ts @@ -0,0 +1,33 @@ +export interface MeshtasticChannelListItem { + index: number; + name: string; +} + +/** + * Resolve the Meshtastic channel list ChatPanel should show. + * + * `meshtasticIdentityId` is nulled for the brief gap between disconnect and + * wire-effect rebind. Without bridging that gap, the caller's hook-local + * `hookChannels` placeholder (a single generic "Primary" entry) would + * transiently replace the real device channel list, tripping ChatPanel's + * invalid-selection clamp and permanently resetting the user's channel + * selection on every reconnect. `lastKnownChannels` (the caller's cache of + * the most recently committed real channel list) bridges that gap. + * + * A pure function so this logic can be unit-tested directly without mocking + * the rest of the runtime hook; the caller (`useMeshtasticRuntime`) owns + * updating the cache, and must do so outside of render (e.g. in an effect) — + * React may replay or discard a render, so mutating a ref from inside the + * `useMemo` that calls this function would leak uncommitted state. + */ +export function resolveMeshtasticChannels(params: { + meshtasticIdentityId: string | null; + deviceRecordChannels: MeshtasticChannelListItem[] | undefined; + hookChannels: MeshtasticChannelListItem[]; + lastKnownChannels: MeshtasticChannelListItem[]; +}): MeshtasticChannelListItem[] { + const { meshtasticIdentityId, deviceRecordChannels, hookChannels, lastKnownChannels } = params; + if (deviceRecordChannels && deviceRecordChannels.length > 0) return deviceRecordChannels; + if (!meshtasticIdentityId && lastKnownChannels.length > 0) return lastKnownChannels; + return hookChannels; +} diff --git a/src/renderer/runtime/useMeshtasticRuntime.reconnect-hardening.test.ts b/src/renderer/runtime/useMeshtasticRuntime.reconnect-hardening.test.ts index ac66bc47f..b521a14ab 100644 --- a/src/renderer/runtime/useMeshtasticRuntime.reconnect-hardening.test.ts +++ b/src/renderer/runtime/useMeshtasticRuntime.reconnect-hardening.test.ts @@ -396,4 +396,41 @@ describe('useMeshtasticRuntime Linux BLE reconnect peripheral id backfill', () = /pushMqttChannelKeys\(\);\s*\}, \[channelConfigs, mqttStatus, pushMqttChannelKeys\]/, ); }); + + it('resolves channels via the pure resolveMeshtasticChannels selector, caching post-commit only', () => { + // meshtasticIdentityId is nulled on every disconnect (cleanupSubscriptions) and only + // restored once wire subscriptions rebind, briefly making the resolved channel list + // fall through to the single-channel `channels` placeholder default — which used to + // clobber ChatPanel's channel selection on every reconnect. resolveMeshtasticChannels + // (behavior covered directly in resolveMeshtasticChannels.test.ts, no mocking needed) + // bridges that gap via a cache; the cache write must stay out of the useMemo that + // calls it (React may replay/discard a render, leaking uncommitted channels) and live + // in an effect instead. + expect(SOURCE).toContain('resolveMeshtasticChannels('); + expect(SOURCE).toContain('lastKnownChannelsRef'); + const resolvedChannelsIdx = SOURCE.indexOf('const resolvedChannels = useMemo('); + expect(resolvedChannelsIdx).toBeGreaterThan(-1); + const resolvedChannelsBody = SOURCE.slice(resolvedChannelsIdx, resolvedChannelsIdx + 400); + expect(resolvedChannelsBody).not.toContain('lastKnownChannelsRef.current ='); + expect(resolvedChannelsBody).toContain('lastKnownChannels: lastKnownChannelsRef.current'); + + const cacheEffectIdx = SOURCE.indexOf( + 'useEffect(() => {\n if (meshtasticDeviceRecord?.channels.length) {\n lastKnownChannelsRef.current = meshtasticDeviceRecord.channels;', + ); + expect(cacheEffectIdx).toBeGreaterThan(resolvedChannelsIdx); + }); + + it('clears the carried-forward channel list on explicit (user-initiated) disconnect only', () => { + // Bridging the reconnect gap is only correct while an auto-reconnect is actually + // in flight for the *same* device. A user-initiated disconnect (no reconnect + // planned) must not leave the disconnected device's channel list lingering + // indefinitely — cleanupSubscriptions() also runs mid-reconnect, where the flag + // is still false and the ref must be left alone. + const cleanupIdx = SOURCE.indexOf('const cleanupSubscriptions = useCallback('); + expect(cleanupIdx).toBeGreaterThan(-1); + const cleanupBody = SOURCE.slice(cleanupIdx, cleanupIdx + 1200); + expect(cleanupBody).toMatch( + /meshtasticExplicitDisconnectRef\.current[\s\S]*?lastKnownChannelsRef\.current = \[\]/, + ); + }); }); diff --git a/src/renderer/runtime/useMeshtasticRuntime.ts b/src/renderer/runtime/useMeshtasticRuntime.ts index ebf616ea7..6cbbd2ce0 100644 --- a/src/renderer/runtime/useMeshtasticRuntime.ts +++ b/src/renderer/runtime/useMeshtasticRuntime.ts @@ -143,6 +143,7 @@ import { meshtasticXmodemDownload, meshtasticXmodemUpload, } from '../lib/meshtastic/meshtasticXmodemTransfer'; +import { resolveMeshtasticChannels } from '../lib/meshtastic/resolveMeshtasticChannels'; import { setRemoteAdminReadsActive } from '../lib/meshtasticBacklogUtils'; import { setMeshtasticConnectedMyNodeNum } from '../lib/meshtasticConnectedNodeRef'; import { @@ -458,6 +459,14 @@ export function useMeshtasticRuntime() { // Nodes heard via RF this session — prevents MQTT-only flag from being set const rfHeardNodeIds = useRef>(new Set()); const lastRfSelfNodeIdRef = useRef(loadPersistedLastRfSelfNodeId()); + /** + * Last real (device-record-backed) channel list, carried across the brief gap + * between disconnect and wire-effect rebind (`meshtasticIdentityId` transiently + * null) so `resolvedChannels` doesn't collapse to the single-channel `channels` + * placeholder default mid-reconnect — that transient collapse was clobbering + * ChatPanel's channel selection on every reconnect. + */ + const lastKnownChannelsRef = useRef<{ index: number; name: string }[]>([]); const virtualNodeIdRef = useRef(getOrCreateVirtualNodeId()); // MQTT-only fallback; RF sessions use the ingest session's shared RF/MQTT registry. const mqttOnlySeenPacketIdsRef = useRef>(new Map()); @@ -867,6 +876,12 @@ export function useMeshtasticRuntime() { meshtasticIdentityIdRef.current = null; meshtasticDriverConnectedRef.current = false; setMeshtasticIdentityId(null); + if (meshtasticExplicitDisconnectRef.current) { + // User-initiated disconnect (no auto-reconnect planned) — stop bridging the + // now-disconnected device's channel list; a genuine reconnect gap (flag still + // false here) keeps it so ChatPanel's selection survives the gap instead. + lastKnownChannelsRef.current = []; + } for (const unsub of unsubscribesRef.current) { try { unsub(); @@ -4331,11 +4346,24 @@ export function useMeshtasticRuntime() { return queueStatus; }, [meshtasticIdentityId, queueStatus, meshtasticConnectionFromStore]); - const resolvedChannels = useMemo(() => { - if (!meshtasticIdentityId) return channels; - if (meshtasticDeviceRecord?.channels.length) return meshtasticDeviceRecord.channels; - return channels; - }, [meshtasticIdentityId, channels, meshtasticDeviceRecord]); + const resolvedChannels = useMemo( + () => + resolveMeshtasticChannels({ + meshtasticIdentityId, + deviceRecordChannels: meshtasticDeviceRecord?.channels, + hookChannels: channels, + lastKnownChannels: lastKnownChannelsRef.current, + }), + [meshtasticIdentityId, channels, meshtasticDeviceRecord], + ); + // Cache the last known real channel list post-commit — never mutate the ref + // inside the useMemo above; React may replay or discard a render, which + // would leak an uncommitted device's channels into the cache. + useEffect(() => { + if (meshtasticDeviceRecord?.channels.length) { + lastKnownChannelsRef.current = meshtasticDeviceRecord.channels; + } + }, [meshtasticDeviceRecord]); const resolvedChannelConfigs = useMemo(() => { if (!meshtasticIdentityId) return channelConfigs;