Repository navigation
fix(channels): keep client-only state (PSK selection, My Channels, unread) across a channel-list refresh - #153
Merged
Conversation
Relates to #152. loadChannels() replaces the channel list with the server snapshot and carries nothing across. These tests drive the real loadChannels(), the real WS path and the real init() handlers (region change, show-encrypted toggle, shared-channel approval) through the existing test hooks, and a Playwright check adds a PSK through the Add Channel modal and switches region at desktop and mobile width. On master, 11 of 15 unit tests and 4 of 6 E2E steps fail with the reported symptoms: the PSK selection is cleared and the URL rewritten to #/channels, My Channels and labels vanish, unread and the user:* preview reset, and a WS update that lands during an in-flight request is lost. The four passing unit tests are guards for the fix (omitted rows are not resurrected, a stale client row does not beat a newer snapshot). Registered in the existing unit and Playwright steps of deploy.yml and in test-all.sh. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DxHDRtHRCQxS6AzzDXXSJi
Relates to #152. loadChannels() replaced the channel list with the server snapshot and carried nothing across, so a region change, the show-encrypted toggle or a shared-channel approval closed an open PSK conversation, dropped My Channels and reset unread badges and user:* previews. loadChannels() now runs: fresh snapshot (copied, so the api() cache is not mutated) -> mergeApprovedChannels -> mergeUserChannels -> mergeClientChannelState -> sort -> render -> reconcile. The new pure helper carries unread (explicit 0 included), userAdded and userLabel by hash for server rows and user:* rows alike, and never re-creates a row the fresh list lacks, so the region filter keeps working. A label just re-read from storage wins over the old one. Live activity (lastActivityMs, lastSender, lastMessage, messageCount, moved together) is carried only when the WS path stamped the row with a local sequence number newer than the one loadChannels() sampled when its request started. No browser/server time comparison, so clock skew cannot decide it. user:* rows exist only in the tab and always keep theirs. onApproved now just calls loadChannels(): its trailing mergeUserChannels()/render ran after the reconcile had already closed the PSK conversation, and is redundant now. init() keeps its own mergeUserChannels()/render: a no-op after a successful load, but the only thing that lists My Channels when /channels fails (covered by a test). Test follow-ups: wait for the decrypt pass before the E2E pane snapshot, a cross-realm array assertion, and tests for init()'s failure path and for a stored label winning over the previous list's. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DxHDRtHRCQxS6AzzDXXSJi
This was referenced Oct 1, 2026
A region change while a PSK (user:* or key-matched) channel's decrypt
fetch is still in flight left the message pane stuck on "Decrypting
messages…": the in-flight decrypt's own staleness check discards its
result once the region changes under it, and the region handler's
refreshMessages({regionSwitch:true}) has no REST messages to refetch for
an encrypted row, so it just returned without ever restarting the
decrypt. The same gap could also leave a finished decrypt showing the
previous region's messages.
regionChangeHandler now re-runs selectChannel(selectedHash) for an
encrypted selection instead of refreshMessages() — it redoes the key
lookup and decrypt fetch for the new region without closing the
conversation (selectedHash/URL stay put).
Test: new unit test drives selectChannel() with a real AES-128-ECB +
HMAC-SHA256 encrypted packet (Node's crypto, matching the scheme
documented in channel-decrypt.js) and an in-flight /packets fetch,
switches region mid-flight, and asserts the pane ends up with the
new-region message, not stuck on "Decrypting…" and not showing the old
region's message. Confirmed red by reverting this fix (pane stays stuck
on "Decrypting…").
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Vxw6Ez4BQJo7w99BQqxnji
mergeClientChannelState() carried prev.userAdded/prev.userLabel forward from the in-memory previous channel list, and the remove-key handler's server-known-channel branch unmarked userAdded but never cleared userLabel. Removing a key from a server-known channel therefore left a stale userLabel on the in-memory row; the next refresh's merge carried it back onto the fresh row (mergeUserChannels() only (re)derives it for keys still in storage, so a removed key's label was never otherwise overwritten), making the removed label resurface indefinitely. mergeUserChannels() already re-derives both userAdded and userLabel from storage on every load, before mergeClientChannelState() runs — so the merge never needed to carry them from prev at all. Drop that carry-over and make storage the single source of truth; also clear userLabel in the remove handler itself so the UI doesn't show it for one extra render. Extracted the remove-button logic into removeUserChannelKey() so it's directly testable. Tests: a new regression test adds a key+label to a server-known channel, removes it through the real handler, and asserts the label and My Channels membership don't resurrect across two loadChannels() refreshes. Updated the existing mergeClientChannelState pure-helper test, whose assertions encoded the old (buggy) carry-over contract. Confirmed red against two separate mutants: reverting the merge-helper change alone (caught by the pure-helper test) and reverting the remove-handler's label clear alone (caught by the new regression test). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vxw6Ez4BQJo7w99BQqxnji
mergeUserChannels() matches a stored PSK key against a server-known or newly-approved shared channel by name (not just by hash), annotating the existing row with userAdded instead of creating a separate user:* row. If that PSK's conversation was already open under its user:* hash, the hash simply disappears from the fresh channel list on the next refresh (a region change, or a shared-channel approval), even though the conversation is still live — reconcileSelectionAfterChannelRefresh() then treats the missing hash as "channel gone" and closes it: clears the selection, empties messages, and rewrites the URL to #/channels. reconcileSelectionAfterChannelRefresh() now checks, before closing a missing user:* selection, whether a userAdded row with the same PSK name exists in the fresh list; if so it remaps selectedHash to that row and updates the URL via replaceState, without touching messages. Tests: two new regression tests cover both triggers — a region switch that reveals a same-named server channel, and approving a shared channel with the same name as the open PSK — each asserting the selection remaps (not closes), the URL updates, and messages are left alone. Confirmed red by reverting the reconcile change (both tests fail: selection closes to null instead of remapping). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vxw6Ez4BQJo7w99BQqxnji
…utant The mutant "drop _wsSeq from the object literal when processWSBatch() creates a brand-new channel row" survived the existing suite: every _wsSeq regression test exercised a row that already existed in channels (the ch._wsSeq = ++wsActivitySeq branch), never the push() branch for a channel seen for the first time over WS. Add a test: a live message for a channel with no existing row creates one via the WS path while a concurrent loadChannels() is in flight, then an older snapshot for that same channel resolves. Without the new row's _wsSeq stamp, mergeClientChannelState() has no way to tell the freshly live row is newer than the request, so the older snapshot would win. Confirmed red by removing the field from the push() literal. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vxw6Ez4BQJo7w99BQqxnji
…nnels-client-state
…region-switch race The existing #152 E2E (test-channels-client-state-152-e2e.js) uses a PSK key that matches no fixture packets, so it never exercises an in-flight client-side decrypt — it can't catch F1 (region switch while decryptAndRender()'s /api/packets fetch is still pending). Add a dedicated E2E test that seeds two genuinely client-decryptable GRP_TXT packets (real AES-128-ECB + HMAC-SHA256, matching the scheme in channel-decrypt.js) directly into a temp copy of the fixture DB, under a PSK key the server never sees — one observed only from an SJC observer, one only from SFO, so a region-scoped /api/packets?region=<R> fetch returns exactly one. It delays the first /packets response ~3s, narrows the region filter mid-decrypt, and asserts the pane ends up showing the new region's message (not stuck on "Decrypting…", not the stale one), with the conversation still open. Runs at desktop and mobile. Since the server refuses to start against an unmigrated DB and writes must not go through the read-only server connection, the test briefly runs the real ingestor (against an in-process fake MQTT broker, no real traffic) to apply schema migrations, stops it, seeds the packets, then starts the read-only server — consistent with the read/write separation invariant. Confirmed red against the pre-F1 code (10 passed, 4 failed — pane stuck on "Decrypting…" and/or showing it instead of the new-region message) and green against the current (fixed) code (14 passed, 0 failed), both independently re-run. Registered in .github/workflows/deploy.yml next to test-channel-proposals-e2e.js (same own-server-and-ingestor pattern). Not added to test-all.sh, which only runs unit tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vxw6Ez4BQJo7w99BQqxnji
Owner
Author
|
Review feedback addressed (commit
Also merged current Full test counts and per-fix detail are in the updated PR description. One out-of-scope finding (a possible read/write-separation invariant violation in Generated by Claude Code |
A region switch, the show-encrypted toggle, or a shared-channel approval can all trigger loadChannels() -> reconcileSelectionAfterChannelRefresh(), which remaps an open user:* PSK conversation to a same-named row mergeUserChannels() just matched by name (#152 F3, round 2). If a decrypt for the old user:* hash was still in flight at that exact moment, the remap changes selectedHash out from under it: the in-flight fetch's own staleness check (isStaleMessageRequest) discards its result once it resolves, and nothing else restarts the fetch for the remapped hash — the pane is left stuck on "Decrypting messages…" forever. Track whether a decrypt is pending for the current selection (messageLoadPending, set/cleared around decryptAndRender()). When reconcile remaps a selection while that flag is set, it now also calls selectChannel(remapped.hash) to restart loading for the new hash. A quiet remap (no decrypt in flight) still leaves messages untouched, preserving the round-2 F3 invariant. Tests: a new unit test starts a real decrypt (in-flight /packets fetch) on a user:* hash, triggers a remap to a server-known row mid-flight, and asserts the remapped channel's own messages load (not stuck on "Decrypting…"). A second test guards the quiet-remap case. Confirmed red by reverting the reconcile change (pane stays stuck on "Decrypting…"). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vxw6Ez4BQJo7w99BQqxnji
fetchAndDecryptChannel()'s cache key was the channel name alone, with no
region component, even though the fetched/decrypted message set depends
entirely on which observers the current region filter includes. This let
a cache entry primed under one region answer a fetch for a different
region:
- Zero candidates for the current region fell through to `return {
messages: cachedMsgs, empty: true }`, showing a foreign region's
messages instead of an empty list (e.g. All->OAK showed the SJC+SFO
messages cached under "All"; SJC->MRY showed SJC's message).
- The delta path's "0 new candidates since lastTs" short-circuit
(`newCandidates.length === 0`) trusted the cache's count/lastTs across
regions too: a different region with the same candidate count but an
older timestamp returned the old region's cached message outright.
- A stale (superseded) decrypt could still write its results into the
cache after being superseded, since the write sites had no visibility
into request staleness at all.
Fixes: the cache key now includes the region param (sorted, so selection
order doesn't fragment it); a zero-candidate result always renders as an
empty list, never leftover cached content; every cache write goes
through setCacheIfFresh(), which checks the caller-supplied isStale()
(backed by isStaleMessageRequest()) first. Also capped the number of
distinct cache entries (MAX_CACHE_KEYS, channel-decrypt.js) since
region-scoped keys mean one channel can now occupy several, evicting the
least-recently-written ones.
Tests: three new unit tests cover cross-region isolation (All->OAK),
the same-count delta-path pitfall (SJC->MRY), and that a superseded
decrypt's evidence never reaches the cache. A new E2E step in
test-channels-client-state-152-decrypt-e2e.js switches to a traffic-free
region (OAK) after the existing decrypt has fully settled and asserts an
empty pane, confirmed red against the pre-fix code in a real browser.
Confirmed red for the unit tests too, each against its own mutant
(dropping the region from the cache key; dropping the isStale guard in
setCacheIfFresh).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Vxw6Ez4BQJo7w99BQqxnji
…e E2E harness Two nits in test-channels-client-state-152-decrypt-e2e.js's process lifecycle helpers: - A thrown error in main()'s try block (e.g. the server process never comes up) left `failed` at 0, since no step() had run yet to increment it. The `finally` block's `if (failed) ... else fs.rmSync(dir, ...)` then deleted the log directory anyway, and the FATAL message from the outer catch pointed at log files that no longer existed. Track a `fatal` flag alongside `failed` so logs are kept for either, then re-throw unchanged so the outer catch's behavior is otherwise identical. - waitFor() polled in fixed 200ms steps for up to the full timeout (20s) even when the callback's own check already knew the watched process had exited — the ingestor/server health checks threw on a dead process, but waitFor just caught that and kept retrying instead of stopping. waitFor now takes an optional `proc` and checks its exitCode itself before each poll, failing immediately instead of waiting out the timeout. No behavior change for the success path; these only affect error reporting and how fast a genuine startup failure is surfaced. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vxw6Ez4BQJo7w99BQqxnji
…nnels-client-state
Owner
Author
|
Review feedback addressed (commits
All three mutants for this round (drop the N1 reselect, drop the region from the cache key, drop the staleness gate on cache writes) are each caught by a dedicated test. Full detail, test counts, and what was not verified (notably: no dedicated E2E for the N1 remap-mid-decrypt race itself, only unit coverage) are in the updated PR description above. Relates to #152 Generated by Claude Code |
…che entry
N2 region-scoped the client-side decrypt cache ("<channel>|<regions>"),
but removeKey() -> clearChannelCache() still only deleted the bare
"<channel>" entry, so removing a PSK left its decrypted plaintext in
localStorage under every region it had been viewed in.
clearChannelCache(name) now deletes "<name>" (the pre-N2 key) and every
key starting with "<name>|", and leaves channels whose name merely shares
the prefix alone. The key format moves into ChannelDecrypt
(channelCacheKey), so the writer in channels.js and the clearer can't
drift apart again.
Tests: two unit tests (raw blob with legacy/All/SJC/SFO,SJC entries plus a
prefix-sharing channel; real decrypts in three regions removed through
the channel-list handler) and a new decrypt-E2E step asserting that no
localStorage value holds the channel's plaintext after Remove, at desktop
and mobile. All red on 9706722.
Relates to #152
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SfbYnGhDMMurKHbweEqTbr
… entries The client-side decrypt cache was capped at 50 entries only. One entry can hold ~365 KiB (1000 messages) against a ~5.2M-character localStorage quota shared with the stored keys and labels, so a full cache made storeKey() and saveLabel() fail silently. - The whole cache blob is kept under 1.5M characters; the least recently used entries (last write, or last read this page load) go first. Read stamps are folded in on the next write, so a read never rewrites the blob. - A QuotaExceededError from the cache write evicts the oldest entry and retries; an entry that can never fit drops the blob instead. setItem() stores the whole new blob or keeps the old one, so it is never half-written. - Keys and labels go through setItemMakingRoom(): on a quota failure they evict decrypt cache until the write fits. - Pre-N2 entries (keyed by channel name alone, no "|") are dropped once, marked by corescope_channel_cache_v=2. Each entry is serialised once and joined, so the budget costs no extra stringify. Measured in Node with 12 channels x 1000 messages written in turn: setCache 23.0 -> 14.1 ms, getCache 13.0 -> 5.9 ms on average, since the blob no longer grows past the budget. Tests (all red on 9706722): budget + eviction order, read keeps an entry alive, quota evict-and-retry with a never-fitting entry, storeKey/labels under a full quota, one-time legacy migration. Mutants killed: no eviction, reversed eviction, no quota retry, reads not counted, keys not making room, no migration, migration on every load. Relates to #152 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SfbYnGhDMMurKHbweEqTbr
N1's "a decrypt is pending" flag was one shared boolean, set only inside decryptAndRender(). That left three races: - S1: decrypt A, a region switch starts decrypt B, A finishes and clears the flag while B still runs; a remap then saw no pending load and left the pane on "Decrypting messages…". - S2: a remap while selectChannel() still awaited computeChannelHash() happened before the flag was set at all. - S3: a superseded selectChannel() resuming after computeChannelHash() still painted "Decrypting messages…" over the newer request's view (e.g. a REST refresh after a region switch revealed a same-named server channel). selectChannel() now sets messageLoadPending to its own request id before its first await and only the owning request clears it (the body moves into loadSelectedChannel(request, ...)). reconcileSelectionAfterChannel- Refresh() restarts loading when the flag equals the current messageRequestId, and decryptAndRender() returns early for a stale request instead of writing "Decrypting messages…". Tests S1, S2 and S3 are red on 9706722. Mutants killed: a global flag cleared by any request (S1), the flag set after the first await (S2), no stale check before "Decrypting…" (S3), no restart on remap (N1, S1, S2). Relates to #152 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SfbYnGhDMMurKHbweEqTbr
…xed flushes S1's "decrypt B is in flight" precondition counted /packets fetches after a fixed flush(300). Those fetches only start once computeChannelHash()'s Web Crypto digest resolves, which runs off the event loop and can outlast the flushes on a busy machine — seen failing once in about eight runs while mutation-testing R4-4. A settle(cond) helper now flushes until the condition holds or ~2s of real time pass, and S1 uses it for its in-flight preconditions and for the remapped load. Production code is unchanged. Relates to #152 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SfbYnGhDMMurKHbweEqTbr
… entry When the current region's decrypt fetch finds zero candidates, N2 renders an empty pane but left that region's cache entry in place, so the next visit flashed the outdated history (onCacheHit) before its own fetch answered. fetchAndDecryptChannel() now deletes the "<channel>|<regions>" entry in that case, unless the request is stale by then (a newer request owns the cache). ChannelDecrypt gains deleteCache(key) for removing one entry. Test (red on 9706722): a stale zero-candidate answer leaves the entry, a current one drops it, and the next visit shows nothing outdated while its fetch is in flight. Mutants killed: zero candidates falling back to the cache, no delete, deleting even when stale. Relates to #152 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SfbYnGhDMMurKHbweEqTbr
…ed region key - The decrypt E2E's waitFor() and stopProcess() only checked exitCode. A child killed by a signal (SIGKILL, SIGSEGV) has exitCode null and signalCode set, so waitFor() polled out its whole timeout and stopProcess() waited forever for an 'exit' that had already fired. Both now use hasExited() (exitCode or signalCode), and the early-exit error names the signal. - The E2E file requires node:sqlite and playwright where they are used, and exports its process helpers, so the unit suite can test them. - Unit tests: waitFor() rejects at once for SIGKILL and SIGSEGV, and stopProcess() returns at once for a signal-killed child (both red on 9706722); SJC,SFO and SFO,SJC share one decrypt cache entry (pins existing behaviour — green on 9706722, red on the unsorted mutant). Relates to #152 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SfbYnGhDMMurKHbweEqTbr
…channelCacheKey R4-1 moved the decrypt cache key format into ChannelDecrypt (channelCacheKey), which fetchAndDecryptChannel() now calls. The two delta-cache tests in test-channels-observed-path-hash-size.js replace ChannelDecrypt with a partial stub, so they failed with "ChannelDecrypt.channelCacheKey is not a function" (11/11 -> 9/11). The stub now provides it; the tests are unchanged otherwise and pass 11/11. Relates to #152 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SfbYnGhDMMurKHbweEqTbr
Owner
Author
|
Review feedback addressed (commits
Local results:
Relates to #152 Generated by Claude Code |
This was referenced Oct 1, 2026
adminopenclaw8-sketch
pushed a commit
that referenced
this pull request
Oct 2, 2026
…ests Both Kpa-clawbot#1468 tests snapshotted window._channelsGetStateForTest() as soon as the WS test hook existed. channels.js sets that hook synchronously in init(), before loadChannels() has an /api/channels answer, so `before` could read 0 channels. When the response then landed between the two evaluate() calls, `after` read the full list and the orphan test failed with before=0, after=5 although the orphan message was dropped (CI run 37002943213). With a delay on /api/channels the same race fails on #153's head too, and it also hits the control test: a WS-born channel pushed while the request is in flight is not in the snapshot, and mergeClientChannelState() only enriches rows the snapshot has. waitForChannelListSettled() waits until #chList has rendered the list (no .ch-loading), the count is > 0 and unchanged for 300 ms; both tests take their `before` snapshot only after that. Test-only change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 3, 2026
Conflicts: - .github/workflows/deploy.yml: master replaced the inline JS unit list with `sh test-all.sh` (#174); take master's step. The #152 unit file now runs through test-all.sh. - test-all.sh: take master's `run` form and keep test-channels-client-state-152.js after test-channels-observed-path-hash-size.js. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
Report — CS-pve-agent1 PR#153 master-merge — head 3ab8113Status: master merged (merge commit, no rebase); the conflicts are in CI lists only; local unit and E2E green; CI: green (run 37198425624). Merged master (commit Evidence tags: [T] = test run locally, [A] = analysis of code or diff, [K] = CI result. Conflicts and how they were resolved
Local tests on
|
| Suite | Result |
|---|---|
sh test-all.sh |
215 passed, 0 failed (215 files) [T] |
node test-frontend-helpers.js |
707 passed, 0 failed [T] |
test-channels-client-state-152.js |
42/0 [T] |
All other unit test-channel*.js (22 files) |
all 0 failed. test-channel-colors, -psk-ux, -ux-followup and -ux-round2 are green now; master fixed the old baseline failures. [T] |
test-channels-client-state-152-e2e.js |
6/0 [T] |
test-channels-client-state-152-decrypt-e2e.js (own server + ingestor) |
18/0 [T] |
test-channel-proposals-e2e.js (own server + ingestor) |
23/0 [T] |
test-e2e-playwright.js |
132/135; 3 are hard-coded or fixture skips, the same as in CI [T] |
Other channel E2E: test-channel-{decrypt,fluid,issue-1087,issue-1111,modal,qr,color-picker}-e2e.js, test-channels-{add-modal,list-render,observed-path-hash-size,selection-flow,share-color,ws-batch,ws-race-1498}-e2e.js, test-issue-1224-…, test-issue-1367-… |
all 0 failed [T] |
Known red baseline: none found in these suites.
CI (run 37198425624, head 3ab81130, conclusion: success)
| Job | Result |
|---|---|
| ✅ Go Build & Test | success. test-all.sh: 215 passed, 0 failed; test-channels-client-state-152.js 42/0; the preflight XSS --diff step passed [K] |
| 🎭 Playwright E2E Tests | success. test-e2e-playwright.js 132/135 (3 skips); test-channels-client-state-152-e2e.js 6/0; test-channel-proposals-e2e.js 23/0; test-channels-client-state-152-decrypt-e2e.js 18/0 [K] |
| 🏗️ Build & Publish Docker Image | success [K] |
| 📦 Release Artifacts, 🚀 Deploy Staging, 📝 Publish Badges & Summary | skipped (fork guards / PR event) [K] |
🤖 Generated with Claude Code
Collaborator
Review — CS-Minimax PR#153 kanalpakke — head 3ab8113Dom: APPROVE Independent, read-only review of head Evidence tags: [T] run here, [A] analysis of the source, [K] taken from the author's report or CI, not re-run. Findings
1. The master merge
|
| Blob | Value |
|---|---|
Old merge base 727efca0 |
10b9e241 |
origin/master 376d51c8 |
10b9e241, identical to the base |
Old head dde4fc98 |
afb14ea5 |
New head 3ab81130 |
afb14ea5, identical to the old head |
Merged tree 0f293e20 |
afb14ea5 |
- Master has not touched
channels.jssince 1 October. The same holds forchannel-decrypt.js(base and masterb7456062) andchannel-proposals.js(5f49499eeverywhere). - So no master change to the channels page can have been lost, and the new head's channel code is byte-identical to the round-4 head. [T]
Master's channel-related work.
autoApprove(feat(channels): opt-in auto-approval for new shared channels #196) lives incmd/ingestorandinternal/channelregistry. The frontend needs no change for it (see F1). The proposals E2E, including the auto-approve steps, is green on the fix(channels): drop stale channel-list responses and show the unread badge on mobile (#154, #155) #225 merged tree (23/0). [A][T]- The deep links of fix(analytics): escape-safe ?tab= lookup and withQuery contract (#193) #194/feat(analytics): deep-link Scopes sub-tabs and window (#205) #206 and the packets URL of fix(packets): URL and modal leftovers from #167 (#180) #204 are outside
channels.js. [A] - The 48 px channel buttons of the 2078 port (
06f53ab1) are CSS outside these files, and are present in the merged tree. [A]
Conflict resolution (deploy.yml, test-all.sh). The unit file now runs through master's test-all.sh (run test-channels-client-state-152.js), and the two E2E lines in the Playwright step are kept. The fork guards are still 9/1. [T]
2. #152: client state survives every trigger
- Order.
loadChannels()does: fresh copy →mergeApprovedChannels→mergeUserChannels()→mergeClientChannelState()→ sort → render → reconcile.- The merge never resurrects a row the fresh list lacks.
- Unread, including an explicit 0, and live activity are carried by the WS sequence, not by comparing times. [A]
- Region change and show-encrypted, in the browser (own probe; full table in the fix(channels): drop stale channel-list responses and show the unread badge on mobile (#154, #155) #225 comment):
- With a PSK conversation open, both triggers keep the URL, header, selection and the labelled My Channels row.
- An unread count of 3 on the PSK row survives both triggers.
- Master closes the conversation and drops the row. [T]
- Shared-channel approval.
- The unit test
shared-channel approval (onApproved) keeps the open PSK conversation, one PSK rowis green. - Mutant Mu152d (no
mergeUserChannels()in the load) fails it, and fails the region and show-encrypted tests too. [T] - The auto-approve variant (feat(channels): opt-in auto-approval for new shared channels #196) never triggers a refresh in the suggesting tab, so it cannot close the conversation (F1). [A]
- The unit test
- E2E on this merged tree:
test-channels-client-state-152-e2e.js6/0,test-channel-issue-1111-e2e.js2/0,test-channels-selection-flow-e2e.js8/0,test-channel-decrypt-e2e.js15/0. [T]
Tests on this merged tree
sh test-all.sh 215/0, node test-frontend-helpers.js 707/0, test-channels-client-state-152.js 42/0, other channel unit files green. [T] The #152 mutants Mu152a–d are all killed (see #225).
Security and rules
scripts/check-xss-sinks.sh --diffagainst master: exit 0. [T]- Fork guards 9/1. [T]
- No closing keywords. [T]
- All commits are by
dborup <kontakt@meshview.dk>. [T]
Not verified
The approval trigger was not driven in a browser with a PSK open; it is covered by the unit test. Real devices were not tested.
The head was 3ab811300690281699654106d9ca86cd46ca708e before and after this review. The PR is still a draft and was not modified.
Generated by Claude Code
dborup
marked this pull request as ready for review
October 4, 2026 13:23
3 tasks
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relates to #152
Problem
loadChannels()inpublic/channels.jsreplaced the channel list with the server snapshot and carried nothing across. Every refresh therefore lost state that only exists in the tab:user:*hash is never in the server snapshot, soreconcileSelectionAfterChannelRefresh()cleared the selection, emptiedmessagesand rewrote the URL to#/channels.user:*rows reset.Triggers: region change, the "show encrypted channels" toggle, and (fork only) shared-channel approval, whose
onApprovedcalledmergeUserChannels()only after the reconcile had already closed the conversation.Design
loadChannels()now runs:fresh snapshot → ChannelProposals.mergeApprovedChannels → mergeUserChannels() → mergeClientChannelState() → sort → renderChannelList() → reconcileSelectionAfterChannelRefresh()api()returns the same cached objects on a TTL hit, andmergeUserChannels()mutates rows. The old code mutated the cache (lastActivityMs,userAdded).mergeClientChannelState(fresh, prev, seqAtRequestStart)is a pure helper in the style ofmergeWsAppendedIntoRest():unread(explicit0included) by hash, for server rows anduser:*rows alike;_wsSeq = ++wsActivitySeq.loadChannels()sampleswsActivitySeqbefore its request, and the previous row'slastActivityMs,lastSender,lastMessageandmessageCountare kept (together) only if its stamp is newer. The browser'sDate.now()is never compared with the server'slastActivity.user:*rows always keep their activity from the previous list. They exist only in the tab;mergeUserChannels()recreates them with the placeholder preview "Encrypted — click to decrypt".mergeUserChannels()runs before the merge so itsuser:*rows get their unread and preview back, and both run before the reconcile.onApprovedandinit()onApproved: the trailingmergeUserChannels()/renderChannelList()is removed. It is idempotent, but now redundant, and it was the ordering bug for this trigger. Covered by the test "shared-channel approval (onApproved) keeps the open PSK conversation, one PSK row" (also checks one/channelsrequest and exactly one PSK row).init(): themergeUserChannels()/renderChannelList()afterloadChannels()is kept on purpose. After a successful load it is an idempotent re-render. But when/channelsfails,loadChannels()merges nothing, and this call is what still lists My Channels. Removing it would change that failure path, so it stays, with an updated comment. Covered by "init(): My Channels still listed when /channels fails" and "init(): a successful load lists each PSK row exactly once".test-channel-issue-1111-e2e.js(case 2 included) stays green: 2/2.How this differs from upstream
Kpa-clawbot/CoreScope#2096Upstream was read as a reference only; nothing was cherry-picked.
prev.lastActivityMs > fresh.lastActivityMs.prev.lastActivityMscomes from the browser'sDate.now()(WS path) andfresh.lastActivityMsfrom the server'slastActivity, so the result depends on clock skew: a browser clock that runs behind drops a WS update that arrived during the request, and one that runs ahead lets a stale client row override a newer snapshot. This PR uses a local sequence instead; both cases are tested with the browser clock 1 h ahead and 1 h behind.user:*rows. Upstream merges the previous state first and callsmergeUserChannels()afterwards, so theuser:*rows it recreates never get their unread count or preview back. HeremergeUserChannels()runs first and the helper coversuser:*rows too.Tests
New unit test
test-channels-client-state-152.js(vm sandbox with the realchannels.js,channel-decrypt.jsandchannel-proposals.js; drives the realloadChannels(), the real WS path and the realinit()handlers through the existing test hooks, plus one new hook_channelsMergeClientChannelStateForTest).New Playwright test
test-channels-client-state-152-e2e.js: adds a test PSK through the Add Channel modal, which opens its conversation, then switches region twice (SJC, then SJC+SFO) with the region pills. It checks the URL, the header, the message pane, the selected row and My Channels (label included). Runs at 1280x800 and 390x844.571fb2c8(final test files)b6521196b6521196merged withorigin/mastere829d1ea(local)The E2E passed 5 runs in a row locally. The 6 unit tests that pass on master are guards for the fix: omitted rows are not resurrected, a stale client row does not override a newer snapshot, and the
init()failure path.Commit history:
1a9d043eadds the failing tests (11/15 unit and 4/6 E2E red on master at that point).b6521196adds the fix plus small test follow-ups: waiting for the decrypt pass before the E2E pane snapshot (it was flaky on mobile without it), a cross-realm array assertion, and three more tests (init()failure path, one PSK row, storage label wins).Mutants
Each was applied to
public/channels.jsand run against the unit test. All 10 are red.mergeUserChannels()after the mergeprev.lastActivityMs > out.lastActivityMs) instead of the sequenceuser:*rowsmergeUserChannels()inloadChannels()user:*activity carried only when WS-stampedExisting suites (local, this branch)
test-channels-merge-1498-unit.jstest-channels-observed-path-hash-size.jstest-channel-proposals.jstest-channel-live-decrypt.js,test-channel-live-decrypt-userprefix.jstest-channel-issue-1087.js,test-channel-issue-1101.jstest-channels-name-escaping.js,test-channels-ping-bot-reply.jstest-channel-modal-ux.js,test-channel-qr*.js,test-channel-decrypt-*.js,test-channel-fluid-layout.js,test-channel-sidebar-layout.js,test-channel-color-picker.jstest-frontend-helpers.jstest-channel-colors.js,test-channel-psk-ux.js,test-channel-ux-followup.js,test-channel-ux-round2.jstest-channel-issue-1111-e2e.jstest-channel-issue-1087-e2e.jstest-channels-list-render-e2e.js,test-channels-selection-flow-e2e.js,test-channels-add-modal-e2e.jstest-channels-ws-race-1498-e2e.js,test-channels-ws-batch-e2e.jstest-channel-proposals-e2e.js(own server + ingestor, approve/revoke flow)Other checks:
scripts/check-xss-sinks.sh --diffis clean (no new HTML sinks), andscripts/check-css-vars.jspasses (no CSS changes).CI registration
.github/workflows/deploy.yml: one line in the JS unit step (aftertest-channels-observed-path-hash-size.js) and one line in the Playwright step (aftertest-channel-issue-1111-e2e.js). No triggers, permissions, jobs or fork guards changed.test-all.sh: one line aftertest-channels-observed-path-hash-size.js.Browser check
test-fixtures/e2e-fixture.db. Screenshots after the region change show the PSK conversation still open ("Issue152 Team — 0 messages") with the row selected under My Channels on desktop, and the detail view still open on mobile. On master the same steps show "Select a channel" / "Choose a channel from the sidebar".unpkg.comis blocked in the sandbox, so the local runs served Leaflet, leaflet.heat and Chart.js from their npm packages through a Playwright route in a preload shim. That is harness only; nothing in the repo.tools/freshen-fixture.shcould not run (nosqlite3CLI); the tests do not depend on fresh timestamps.Perf
Not a hot path.
loadChannels()runs on page load, region change, the toggle and approval. It does one extra O(n) copy and one O(n) Map-based merge over the channel list (hundreds of rows). The WS path adds one integer increment per updated row.wsActivitySeqis a single counter, and_wsSeqis one number per row, dropped with the row.Not verified
scrollLeft50), because the pill row is wider than the 281 px sidebar. It happens on master too.Overlap with other open PRs
test-all.shis also touched bydborup/CoreScope#13(different lines).public/channels.jsor.github/workflows/deploy.yml.Review round 2 — F1/F2/F3 fixes + nit (commits
78bc3a7e..f61a13a6)Independent review found three correctness bugs (no P1) and one surviving mutant in the original change. Fixed as four separate commits on top of the original
b6521196, then merged withorigin/master(nowbe35eefb).F1 (P2): region switch mid-decrypt strands the pane on "Decrypting messages…"
refreshMessages()'sif (selCh && selCh.encrypted) return;early return meant a region change while a PSK channel's client-side decrypt (/api/packetsfetch) was still in flight never restarted it: the in-flight fetch's own staleness check discards its result once the region changes under it (nothing else ever re-fetches), and a since-finished decrypt could keep showing the previous region's messages.Fix: the region-change handler now re-runs
selectChannel(selectedHash)for an encrypted selection instead ofrefreshMessages()— it redoes the key lookup and decrypt fetch for the new region without closing the conversation.Tests:
selectChannel()with a real AES-128-ECB + HMAC-SHA256 encrypted packet (Node'scrypto, matching the scheme inchannel-decrypt.js) and an in-flight/packetsfetch, switches region mid-flight, and asserts the pane ends up with the new-region message.test-channels-client-state-152-decrypt-e2e.js) seeds two genuinely client-decryptable GRP_TXT packets into a temp copy of the fixture DB (one SJC-only, one SFO-only, under a PSK key the server never sees), delays the first/api/packetsresponse ~3s, switches region mid-decrypt, and asserts the pane shows the new region's message — not stuck "Decrypting…", not the stale one. Desktop + mobile.F2 (P3, regression): a removed key's label resurrects on refresh
mergeClientChannelState()carriedprev.userAdded/prev.userLabelforward from the in-memory previous channel list, and the remove-key handler's server-known-channel branch unmarkeduserAddedbut never cleareduserLabel. Removing a key left a stale label on the in-memory row, which the next refresh's merge carried back onto the fresh row (storage no longer has it, so nothing else overwrote it) — the label resurfaced indefinitely.Fix:
mergeUserChannels()already re-derives both fields from storage on every load, before the merge runs, so the merge never needed to carry them fromprevat all — dropped that carry-over, making storage the single source of truth. Also clearuserLabelin the remove handler itself. Extracted the remove-button logic intoremoveUserChannelKey()for direct testability.Tests: new regression test adds a key+label to a server-known channel, removes it through the real handler, and asserts the label and My Channels membership don't resurrect across two
loadChannels()refreshes. Updated the existingmergeClientChannelStatepure-helper test, whose assertions encoded the old carry-over contract. Confirmed red against two separate mutants (reverting the merge-helper change alone; reverting the remove-handler's label clear alone), each caught by a different test.F3 (P2, in scope): a PSK with a server-colliding name gets closed instead of remapped
When a stored PSK's name matches a server-known or newly-approved shared channel,
mergeUserChannels()matches it by name and annotates the existing row instead of creating a separateuser:*row. If that PSK's conversation was open under itsuser:*hash, the hash disappears from the next fresh list even though the conversation is still live, andreconcileSelectionAfterChannelRefresh()treated that as "channel gone" — closing the conversation, clearing messages, rewriting the URL.Fix: before closing a missing
user:*selection,reconcileSelectionAfterChannelRefresh()now checks whether auserAddedrow with the same PSK name exists in the fresh list; if so it remapsselectedHashto that row and updates the URL viareplaceState, without touchingmessages.Tests: two new regression tests, one per trigger — a region switch that reveals a same-named server channel, and approving a shared channel with the same name as the open PSK — each asserting the selection remaps (not closes), the URL updates, and messages are left alone. Confirmed red by reverting the reconcile change (both fail: selection closes to
null).Nit: surviving mutant — no
_wsSeqon a brand-new WS-pushed row"Drop
_wsSeqfrom the object literal whenprocessWSBatch()creates a brand-new channel row" survived the suite — every existing_wsSeqtest exercised a row that already existed, never thechannels.push()branch for a channel seen for the first time over WS. Added a test: a live message for a not-yet-listed channel creates a row via WS while a concurrentloadChannels()is in flight; an older snapshot for that channel then resolves, and only the_wsSeqstamp lets the merge know the live row is newer. Confirmed red by removing the field from the push literal. No production fix needed — the code was already correct.Test counts (this round, actual output)
test-channels-client-state-152.jstest-channels-client-state-152-e2e.jstest-channels-client-state-152-decrypt-e2e.js(new)test-channel-issue-1111-e2e.jstest-channel-proposals.jstest-channel-proposals-e2e.jstest-frontend-helpers.jstest-channel*.jssuites listed earlier in this PR bodycmd/serverGo testsscripts/check-xss-sinks.sh --diff,scripts/check-css-vars.jsTested merged with
origin/master(be35eefb) locally before pushing — no conflicts, same results.CI registration (round 2)
.github/workflows/deploy.yml: one line registeringtest-channels-client-state-152-decrypt-e2e.jsnext totest-channel-proposals-e2e.js(same own-server-and-ingestor pattern;CORESCOPE_SERVER_BIN/CORESCOPE_INGESTOR_BIN/FIXTURE_DB). Not added totest-all.sh, which only runs unit tests.Out of scope, flagged separately
While tracing the decrypt path, found that
cmd/server/hash_migrate.goruns realUPDATE/DELETESQL (lines ~79, 87, 93-94) against the server's own DB connection at startup, which on its face conflicts with the read/write separation invariant (Kpa-clawbot#1283) documented inAGENTS.md("cmd/server/is read-only... must not acquire a write lock"). Not investigated further or touched here — unrelated to #152/#153 and deserves its own look.Review round 3 — N1/N2 fixes + E2E harness nit (commits
371d3f49..69fd262e, merged9706722d)Independent review on top of round 2 (
f61a13a6) found two new P3 bugs and one nit in the test harness. Fixed as three separate commits, then merged withorigin/master(727efca0).N1 (P3): a remap mid-decrypt strands the pane on "Decrypting messages…" forever
A remap of the open
user:*selection (the F3 path — region switch or approval revealing a same-named server channel) could land while that channel's client-side decrypt (/api/packetsfetch) was still in flight. The remap only updatedselectedHashand the URL; it never restarted the fetch. The in-flight request's own staleness check (isStaleMessageRequest) then discarded its result once the hash changed under it, and nothing else ever issued a new request for the remapped hash — the pane stayed on "Decrypting messages…" indefinitely. (Round 1 closed the conversation in this situation instead; F1's fix for the region-switch case didn't cover this path because F1 only handles a hash staying the same while the region changes, not the hash itself changing under an in-flight decrypt.)Fix: a new
messageLoadPendingflag is set for the duration ofdecryptAndRender()'s work.reconcileSelectionAfterChannelRefresh()'s remap branch now checks this flag before reassigningselectedHash; if a decrypt was in flight, it callsselectChannel(remapped.hash)after updating the URL, which redoes the key lookup and decrypt fetch for the remapped hash instead of leaving it orphaned. A quiet remap (no decrypt in flight) is unaffected — messages are left untouched, same as before.Tests: two unit tests — one drives a real encrypted packet fetch, triggers a remap mid-flight, and asserts the remapped channel's message renders (not stuck loading); the other confirms a quiet remap still leaves messages untouched (guards against regressing the F3 invariant). Confirmed red by reverting the reconcile-branch change (pane stuck loading), green after.
N2 (P3, in scope — correct messages for the new region per F1): the decrypt cache leaks messages across regions
ChannelDecrypt's decrypt cache was keyed on the channel name alone, with no region component. Three related leaks followed:setCache()had no staleness check, so a superseded (stale) decrypt could still overwrite the cache with results that no longer matched the current region/request.All → OAKshowed the SJC+SFO messages;SJC → MRYshowed the SJC message; changing region in another tab and reloading with the same candidate count showed the stale region's message.Fix:
fetchAndDecryptChannel()now builds a region key (sorted, comma-joined, so reordering the selection doesn't bust the cache) and includes it in the cache key. The zero-candidates branches now return{ messages: [], empty: true }instead of falling back to the cache. All cache writes are routed through a newsetCacheIfFresh()helper gated on the sameisStaleMessageRequest()checkselectChannel()already uses for rendering, so a superseded decrypt can no longer write its (stale) evidence into the cache.channel-decrypt.js'ssetCache()also gained aMAX_CACHE_KEYS(50) eviction so region-scoped keys can't grow the cache unboundedly over a session.Tests: three unit tests — cache isolation between two regions for the same channel name, a same-candidate-count delta fetch in a different region not showing the old region's message, and a stale (superseded) decrypt not writing into the cache (this one needed a same-timestamp "stale vs. real" setup to isolate the write-gating path from the independent zero-candidates fix). Confirmed red against three separate mutants — no region in the cache key, no zero-candidates-returns-empty, no staleness gate on
setCache— each caught by a different test (see table below).Decrypt-E2E: added a desktop+mobile scenario — after the existing mid-decrypt-then-narrow-to-SFO race settles, deselect SFO and select a region with no traffic for the channel (OAK), and assert the pane goes empty rather than showing the prior region's message. A region-pill helper (
deselectRegion) was added to exercise the multi-select toggle UI correctly (deselecting the only selected pill reverts to "All regions", which marks every pillaria-checked="true"and the__all__pill active — not what reselecting a different single region looks like).A dedicated N1 remap-mid-decrypt E2E scenario was considered but not added — scope note under "Not verified" below.
Nit: E2E harness log preservation + slow failure wait
In
test-channels-client-state-152-decrypt-e2e.js, if server/ingestor startup itself threw (rather than astep()assertion failing),failedwas never incremented, so thefinally-style cleanup deleted the temp directory and the FATAL message pointed at a now-missing log file. Separately,waitFor()polled for its full timeout even when the child process it was watching had already died.Fix:
main()now tracks afatalerror separately fromfailedstep count and preserves the log directory whenever either is set.waitFor(what, fn, timeoutMs, proc)takes the watched child process and throws immediately onceproc.exitCode !== null, instead of waiting out the full 20s timeout against a dead process.Tests: this is harness code, not production logic — verified by deliberately killing the server mid-
waitForand confirming the function returns immediately with a clear error instead of waiting 20s, and by forcing a startup throw and confirming the log directory is preserved and named in the FATAL message.Mutants (round 3)
selectChannel(remapped.hash)reselect on a pending-decrypt remapisStalegate insetCacheIfFreshTest counts (this round, actual output)
test-channels-client-state-152.jstest-channels-client-state-152-decrypt-e2e.jstest-frontend-helpers.jstest-channels-merge-1498-unit.jstest-channels-observed-path-hash-size.jstest-channel-proposals.jstest-channel-live-decrypt.js,test-channel-live-decrypt-userprefix.jstest-channel-issue-1087.js,test-channel-issue-1101.jstest-channels-name-escaping.js,test-channels-ping-bot-reply.jstest-channel-colors.js,test-channel-psk-ux.js,test-channel-ux-followup.js,test-channel-ux-round2.jscmd/serverGo testsscripts/check-xss-sinks.sh --diff f61a13a6,scripts/check-css-vars.jstest-channels-client-state-152-e2e.jsandtest-channel-issue-1111-e2e.jscould not run in this sandbox (missing a Chromiumheadless_shellbinary path at the fixed location these two files expect);test-channel-proposals-e2e.jsself-skipped for the same reason. This is a sandbox binary-path limitation, not a regression — none of these three files were touched this round, and the decrypt-E2E file (which resolves a fallback Chromium path itself) ran clean.Tested merged with
origin/master(727efca0) locally before pushing — no conflicts, same results.CI registration
No changes. Both new tests run inside files already registered in
.github/workflows/deploy.ymlfrom round 1/round 2 (test-channels-client-state-152.jsin the JS unit step,test-channels-client-state-152-decrypt-e2e.jsin the Playwright step) — no new lines needed. Fork guards unchanged:github.repository == 'Kpa-clawbot/CoreScope'still appears 9 times, andgit diff f61a13a6 HEAD -- .github/workflows/deploy.ymlis empty.Out of scope, confirmed untouched
Per review scope: the "0 messages" header text after a remap, F4 (overlapping
loadChannels()calls), and the missing mobile Remove button were not touched this round.Not verified
reconcileSelectionAfterChannelRefresh()/selectChannel()code path directly instead.MAX_CACHE_KEYS— unit-tested with a small synthetic count, not profiled at scale.Relates to #152
🤖 Generated with Claude Code
https://claude.ai/code/session_01Vxw6Ez4BQJo7w99BQqxnji