Skip to content

fix(channels): keep client-only state across a channel-list refresh (#2095) - #2096

Open
efiten wants to merge 1 commit into
Kpa-clawbot:masterfrom
efiten:fix/2095-channels-merge
Open

efiten wants to merge 1 commit into
Kpa-clawbot:masterfrom
efiten:fix/2095-channels-merge

Conversation

@efiten

@efiten efiten commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator

Fixes #2095.

loadChannels() replaced the channels array with the server snapshot and carried nothing across, and mergeUserChannels() only ever ran from init(). So any refresh destroyed every field that exists only in the tab.

No race needed for the worst of it. Change the region filter or toggle show-encrypted with a PSK channel open, and:

  • the My Channels section disappears — renderChannelList() derives it as channels.filter(c => c.userAdded === true), and the server returns neither userAdded nor a user:-prefixed hash
  • every unread badge resets to 0
  • the user's own labels vanish
  • reconcileSelectionAfterChannelRefresh() cannot find the user:* hash in the snapshot, so it nulls selectedHash, sets messages = [], rewrites the URL to #/channels and replaces the open conversation with "Choose a channel from the sidebar"

What this does

mergeClientChannelState(fresh, prev) mirrors mergeWsAppendedIntoRest(), which already does exactly this job for messages (#1498). Same shape: pure, takes both arrays as parameters, returns a fresh array, never aliases or mutates an input.

It carries unread, userAdded and userLabel by hash, and keeps lastActivityMs / lastSender / lastMessage / messageCount when a WebSocket batch landed while the request was in flight and is therefore newer than the snapshot. Those four move together: a sender without its message reads as a different message.

mergeUserChannels() now runs inside loadChannels(), before the render and before the reconcile, so all three call sites get it instead of init() alone.

What it deliberately does not do

It does not resurrect a channel the snapshot left out. A region-filter change legitimately narrows the list, so carrying survivors over would defeat the filter, which is a worse bug than the one being fixed. The helper only enriches rows already present in the fresh snapshot.

That leaves half of finding 2 in the issue unfixed: a channel pushed by the WebSocket handler during the initial in-flight window is still dropped. That one self-heals on the channel's next packet, and the reverted preview line is overwritten by the next WS batch. Fixing it properly needs a way to tell "dropped because the snapshot is stale" from "dropped because the filter excludes it", which is a larger change than this.

Tests

tests/unit/test-issue-2095-channels-client-state.js, 11 cases, registered in test-all.sh.

Part 1 exercises the helper directly. Part 2 drives the real loadChannels() through the existing _channelsLoadChannelsForTest hook with a stubbed api(), which is what proves the helper is wired in rather than merely defined.

Verified by mutation. With the wiring removed from loadChannels() but the helper left in place, the three reproduction cases fail with exactly the reported symptoms:

FAIL  a refresh keeps the My Channels rows
      My Channels lost on refresh (got ["public1"])
FAIL  a refresh keeps unread badges
      the unread badge reset to 0 on refresh   undefined !== 7
FAIL  a refresh does not close an open PSK conversation
      the open PSK channel was deselected      + null  - 'user:MyPSK'

The fourth case, "a refresh still drops a channel the server filtered out", stays green throughout, so the fix cannot be defeating the region filter.

Full frontend suite green: sh test-all.sh exits 0.

Not done

No browser validation. This is frontend JS covered by unit tests that call the production function through its own hook, but I did not run Playwright against a server, so I am not claiming a browser check I did not do.

init() still calls mergeUserChannels() and renderChannelList() after loadChannels() resolves, which is now redundant and costs one extra full sidebar render per page load. Left alone deliberately: it is idempotent, and the comment there records the regression it was added to fix (test-channel-issue-1111-e2e.js, case 2). Worth removing separately with that e2e test watched.

…pa-clawbot#2095)

loadChannels() replaced the channels array with the server snapshot and carried
nothing across, and mergeUserChannels() only ran from init(). So a region-filter
change or the show-encrypted toggle destroyed every field that exists only in
the tab: the My Channels section, the user's own labels, and all unread badges.
If the selected channel was a PSK row, reconcileSelectionAfterChannelRefresh()
could not find its user:* hash in the snapshot and nulled the selection, emptied
messages and rewrote the URL, closing the open conversation. No race needed.

mergeClientChannelState() mirrors mergeWsAppendedIntoRest(), which already does
this job for messages (Kpa-clawbot#1498). It enriches only rows the snapshot already
contains: carrying a missing row over would resurrect channels the region filter
just excluded, which is worse than the bug being fixed. A row genuinely dropped
by the in-flight race reappears on that channel's next packet.

mergeUserChannels() now runs inside loadChannels(), before the render and before
the reconcile, so every call site gets it rather than init() alone.

Tests drive the real loadChannels() through its existing test hook with a stubbed
api(), so they prove the helper is wired in and not merely present. Verified by
mutation: with the wiring removed the three reproduction tests fail with exactly
the reported symptoms (My Channels reduced to the server rows, unread undefined,
selectedHash null), while the region-filter test stays green.

Not done: no browser validation. The change is frontend JS covered by unit tests
that call the production function; I did not run Playwright against a server.
init() still calls mergeUserChannels() and renderChannelList() after
loadChannels() resolves, which is now redundant. Left alone deliberately: it is
idempotent, and the comment there records a regression it was added to fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

channels: loadChannels() replaces the channel array and discards all client-only state

1 participant