chore: dev-stack switches, shared e2e harness, and desktop shell rules
- `LIVE_RELOAD=false npm run dev` keeps the renderer alive across a machine suspend; the reload client otherwise destroys the session under test. - `dev-peer.sh` plus a separate userdata dir runs a second local peer. - `tools/voice-probe.js` samples peer state and RTP counters from a live window, persisting to localStorage so a renderer reload cannot erase it. - e2e helpers for voice pairs, peer-role election, and a TURN relay. - Electron single-instance and dev-client-load decisions move into rules files with colocated specs.
This commit is contained in:
+154
-7
@@ -25,6 +25,160 @@ Durable rules for AI agents working on this project.
|
||||
|
||||
## Lessons
|
||||
|
||||
### A hold-on-unknown rule needs every attach site behind it [voice] [webrtc] [realtime]
|
||||
|
||||
- **Trigger:** replacing a strict media gate with "hold an established path when nothing confirms the peer", while some fast path still attaches the track without asking the rule.
|
||||
- **Rule:** route every attach site — including the pre-offer shortcut in `createPeerConnection` and every sibling media kind — through the same decision, and refresh all of them on each piece of new evidence.
|
||||
- **Why:** an ungated attach becomes the guess the rule then protects: the track alone reads as an established path, so `hold` keeps sending indefinitely to a peer that never joined the channel. The strict gate used to erase that mistake on the next pass.
|
||||
- **Example:** `MediaManager.mayOpenVoicePathToPeer()` gates the first offer in `create-peer-connection.ts`, `syncCameraRouting()` reuses `decideVoicePathRouting`, and `notePeerVoiceReport()` calls `refreshVoiceRouting()` so a departure report reaches the camera too, not just the mic.
|
||||
|
||||
### Missing gossip about a peer is not evidence it left voice [voice] [webrtc] [realtime]
|
||||
|
||||
- **Trigger:** gating an outgoing media track on the observer's store copy of the *remote* user's `voiceState`, and detaching whenever that copy is absent.
|
||||
- **Rule:** close a negotiated media path only on positive evidence — we left voice, the peer itself reported leaving or another channel, or the connection is gone; treat absence as unknown and hold the path. Opening a path still requires confirmation, so a guess never starts sending the microphone.
|
||||
- **Why:** the signal server broadcasts `user_left` for any socket it declares dead, so a suspend or a flaky hop wipes that copy while the peer is still in the channel. The observer then detached its mic permanently — a silent member with no way back through the UI, not even by toggling mute.
|
||||
- **Example:** `decideVoicePathRouting()` in `toju-app/src/app/domains/voice-session/domain/logic/voice-path-routing.rules.ts`, used by `MediaManager.syncVoiceRouting()` for the mic and `mayHearPeerVoice()` for playback gain.
|
||||
|
||||
### Assert continuity when the state you broke repairs itself [testing] [voice] [verification]
|
||||
|
||||
- **Trigger:** proving a media cut by wiping a peer from the roster and then checking that audio still flows.
|
||||
- **Rule:** when the broken state is refreshed by a periodic message, sample the victim second by second across the window instead of asserting an end state.
|
||||
- **Why:** peers gossip their voice state every 5s (`VOICE_HEARTBEAT_INTERVAL_MS`), so the roster heals moments after the wipe and the mic re-attaches; the end-state check passed against the *unfixed* code and proved nothing. Only a per-second sample showed the peer losing audio.
|
||||
- **Example:** `assertUninterruptedInboundAudio(peer, 10)` in `e2e/tests/voice/roster-loss-preserves-voice.spec.ts` fails on the first silent second; the earlier `assertTwoWayAudio` after the wipe did not.
|
||||
|
||||
### Never test suspend/resume against a live-reloading dev server [testing] [dev-shell] [verification]
|
||||
|
||||
- **Trigger:** suspending the machine to check whether a voice call survives sleep/wake, with the windows served by `ng serve`.
|
||||
- **Rule:** disable the dev server's reload before any suspend/resume test (`LIVE_RELOAD=false npm run dev`), and treat a renderer reload in the results as an invalid run rather than a product finding.
|
||||
- **Why:** `ng serve --ssl` runs Vite over HTTP/2; the suspend destroys that stream, so on resume Vite throws `The stream has been destroyed` from `viteTransformMiddleware` into the error overlay of every window, and its live-reload client reloads the page. The reload re-bootstraps the app out of the call, so the post-resume readings showed a "connected" peer with zero RTP — which looks exactly like a silently dead call but only meant the reloaded app was no longer in voice.
|
||||
- **Example:** `dev.sh` appends `--live-reload=false` when `LIVE_RELOAD=false`; the first P7.4 attempt produced 20 `audio stalled` lines that proved nothing.
|
||||
|
||||
### Keep diagnostic history outside the page you are diagnosing [testing] [verification]
|
||||
|
||||
- **Trigger:** collecting samples into a `window.__probe` array in the DevTools console, then reading them back after the disruptive event.
|
||||
- **Rule:** persist probe samples to `localStorage` (or outside the renderer entirely) and stamp each sample with a per-load id, so a reload keeps the history and becomes visible evidence instead of silent data loss.
|
||||
- **Why:** the event under test is often the very thing that destroys in-heap state; a reload wiped every pre-suspend sample while leaving the old console lines on screen, so the probe looked loaded but `__voiceProbe` was undefined and the baseline was gone.
|
||||
- **Example:** `tools/voice-probe.js` stores samples under `metoyou_voice_probe_v1` and reports `RENDERER RELOADED` when `performance.timeOrigin` changes between samples.
|
||||
|
||||
### Record whether the user is in a call before calling zero RTP a failure [testing] [voice] [verification]
|
||||
|
||||
- **Trigger:** asserting on inbound/outbound audio packets without also recording voice membership and local mic track state.
|
||||
- **Rule:** capture `isVoiceConnected()` and the local audio tracks' `readyState` in the same sample as the RTP counters, and only call a stall a stall when the client is supposed to be in voice.
|
||||
- **Why:** peer connections exist for chat data channels regardless of voice, so "connected with zero audio" is the normal reading outside a call; without the voice flag the two cases are indistinguishable and a healthy app looks broken.
|
||||
- **Example:** `readLocalMedia()` in `tools/voice-probe.js` logs `in-voice mic=live`, and the stall check is gated on `current.voice === 'in-voice'`.
|
||||
|
||||
### Never answer `second-instance` by relaunching the app [electron] [dev-shell]
|
||||
|
||||
- **Trigger:** making a second dev launch reuse the open window by restarting the running instance (`app.relaunch(); app.exit(0)`).
|
||||
- **Rule:** handle a second instance in place — focus and `webContents.reloadIgnoringCache()` — and never relaunch the process from the `second-instance` handler.
|
||||
- **Why:** the relaunched successor inherits the same dev argument and asks for the single-instance lock while the dying parent still holds it, so it is refused as yet another second instance and the pair respawns forever; every generation also exits `0` instead of the launcher's handoff code, so `concurrently --kill-others` tears down `ng serve` and the API server, and an in-flight `loadURL` dies as `ERR_FAILED (-2)` that reads like an unreachable dev server.
|
||||
- **Example:** `resolveSecondInstanceAction()` in `electron/app/second-instance.rules.ts` returns `'reload-existing'`, and `deep-links.ts` reloads instead of relaunching.
|
||||
|
||||
### Never gate a presence indicator on the observer's own participation [ui] [voice] [webrtc]
|
||||
|
||||
- **Trigger:** writing `if (!isUserInCurrentVoiceRoom(...)) return false` before reading a remote user's share/camera state.
|
||||
- **Rule:** decide a remote indicator from the observed user's state alone; keep the observer's own session out of the input entirely.
|
||||
- **Why:** a user sharing alone in a voice channel looked idle to everyone outside it, so nobody could tell there was anything to watch — while the peer plane had already delivered the announcement, because `screen-state` goes to every open data channel and not just voice participants.
|
||||
- **Example:** `shouldShowStreamIndicator()` in `domains/voice-session/domain/logic/stream-indicator.rules.ts`; guarded by `e2e/tests/screen-share/outside-voice-live-indicator.spec.ts`, where the observer never joins voice.
|
||||
|
||||
### `ERR_FAILED (-2)` on a dev `loadURL` usually means aborted, not unreachable [electron] [dev-shell]
|
||||
|
||||
- **Trigger:** blaming the cert or `ng serve` when Electron logs `ERR_FAILED (-2) loading 'https://127.0.0.1:4200'`.
|
||||
- **Rule:** read the rejection stack — `stopLoadingListener` means the navigation was stopped (window destroyed, app exiting), so look for whatever killed the process; `SSL=true` already appends `ignore-certificate-errors`.
|
||||
- **Why:** the cert and the dev server were fine; the app was exiting underneath the load, and chasing TLS wasted the first pass at the bug.
|
||||
- **Example:** `loadDevelopmentClientWithRetry()` in `electron/window/dev-client-load.rules.ts` retries and never throws, so the window still gets its listeners and shows a readable failure page.
|
||||
|
||||
### Pin a chosen media device with `deviceId: { exact }`, never a bare string [webrtc] [media] [electron]
|
||||
|
||||
- **Trigger:** the user picks a different microphone or camera and nothing changes — not mid-call, not after leaving and rejoining voice.
|
||||
- **Rule:** build `getUserMedia` constraints as `deviceId: { exact: id }`, and handle `OverconstrainedError` / `NotFoundError` by retrying once with the system default.
|
||||
- **Why:** a bare `deviceId: id` is an `ideal` constraint, so Chromium may satisfy it with the device it already had; the feature then looks broken while every unit test passes. `exact` makes the request fail loudly instead, which is why it needs the explicit fallback so an unplugged device degrades rather than killing the call.
|
||||
- **Example:** `buildMicrophoneConstraints` in `audio-device-selection.rules.ts` plus the single retry with `SYSTEM_DEFAULT_AUDIO_DEVICE_ID` in `media.manager.ts` `captureMicrophone` and `direct-call.service.ts` `captureCallMicrophone`.
|
||||
|
||||
### A second dev Electron window needs its own `--user-data-dir` [electron] [dev-shell]
|
||||
|
||||
- **Trigger:** launching a second desktop instance for a two-user test; the existing window blinks and reloads and no second window appears.
|
||||
- **Rule:** launch the peer with its own `--user-data-dir` (`npm run dev:peer`), and never launch the desktop shell from an agent shell.
|
||||
- **Why:** Electron's single-instance lock is scoped to the `userData` directory, so a default-directory launch hands its argv to the running instance instead; `tools/launch-electron.js` always appends `--metoyou-dev-reload-existing`, and the `second-instance` handler in `electron/app/deep-links.ts` answers that with `app.relaunch(); app.exit(0)`. Separate data dirs are also what give the two windows separate identities.
|
||||
- **Example:** `dev-peer.sh` — `--user-data-dir="$DIR/.dev-userdata/$PEER_NAME"`.
|
||||
|
||||
### An outage test that only re-checks the end state is not a guard [testing] [verification] [webrtc]
|
||||
|
||||
- **Trigger:** writing or trusting a test that breaks something (kills a server, closes a channel), then asserts the feature works again afterwards.
|
||||
- **Rule:** also assert what must **not** have happened in between — for a call, that the `RTCPeerConnection` was never rebuilt (`countCreatedPeerConnections` unchanged) — and prove the assertion by temporarily injecting the regression.
|
||||
- **Why:** re-checking only the end state passes for a client that tore the call down and rebuilt it, which the user hears as a dropped call. Injecting `peerManager.closeAllPeers()` on signaling reconnect kept every audio and peer-count assertion green; only the connection-count assertion failed.
|
||||
- **Example:** `e2e/tests/voice/recovery-preserves-media.spec.ts` — "The call was never rebuilt behind the user back" compares counts captured before `testServer.kill()`.
|
||||
|
||||
### coturn hands out a relay candidate but refuses loopback peers by default [testing] [webrtc] [turn]
|
||||
|
||||
- **Trigger:** a relay-only test (`iceTransportPolicy: 'relay'`) where candidates gather fine but every peer connection ends up `closed`.
|
||||
- **Rule:** run a local coturn with `--allow-loopback-peers` (plus `--log-file=stdout --verbose`, or `docker logs` stays empty and readiness cannot be observed).
|
||||
- **Why:** without it coturn still allocates and Chrome still reports a `typ relay` candidate, so the failure looks like broken app code rather than a blocked relay; connectivity checks to the other 127.x browser are simply dropped.
|
||||
- **Example:** `e2e/helpers/turn-server.ts` — `--allow-loopback-peers` next to `--relay-ip=127.0.0.1`.
|
||||
|
||||
### Swap a live device with `replaceTrack`; an empty device list is missing evidence [voice] [webrtc] [devices]
|
||||
|
||||
- **Trigger:** a settings picker changes a capture device (mic, camera) while a session is live, or code reacts to `devicechange` by re-reading `enumerateDevices()`.
|
||||
- **Rule:** re-capture, then `replaceTrack` on the existing senders and stop the old track — never tear the session down and rejoin. Treat an empty (or id-less) device list as *no information*: only fall back to the system default when a populated list proves the saved id is gone. Ask for `deviceId` as a preference, not `exact`.
|
||||
- **Why:** `voice-controls.component.ts` called `disconnect()` then `connect()` for a mic change, so every peer saw a leave/rejoin and the user lost the channel; the settings pickers wrote `localStorage` and applied nothing. `enumerateDevices()` returns `[]` before microphone permission is granted and Firefox never lists audio outputs, so "not in the list" would silently reset a valid choice on startup. A plain track swap on an already negotiated sender needs no SDP exchange, so the swap is invisible to peers.
|
||||
- **Example:** `MediaManager.switchInputDevice()` + `resolveAudioDeviceSelection()` / `buildMicrophoneConstraints()` in `domains/voice-session/domain/logic/audio-device-selection.rules.ts`, owned by `VoiceAudioDeviceService`; proven by `e2e/tests/voice/live-input-device-change.spec.ts` (outbound audio keeps flowing, no rejoin broadcast).
|
||||
|
||||
### One owner for a toggle the UI mirrors [voice] [state] [ui]
|
||||
|
||||
- **Trigger:** two surfaces (in-channel controls and a settings modal, a tray and a window) each keep a local `signal` for the same boolean — mute, deafen, camera on.
|
||||
- **Rule:** keep the state where the effect happens and let every surface read it back through a `computed`; never reset a mirror to a hardcoded value on teardown.
|
||||
- **Why:** `MediaManager` owned `isMicMuted` / `isSelfDeafened`, but `voice-controls.component.ts` kept its own copies and reset them to `false` in `disconnect()`, so after leaving voice the button said unmuted while the track was still disabled — and playback was un-deafened behind the user's back.
|
||||
- **Example:** `isMuted = computed(() => this.webrtcService.isMuted())` in `voice-controls.component.ts`; `disconnect()` passes the real state into `voicePlayback.updateDeafened()`.
|
||||
|
||||
### A timed-out sync round is not a clean one [messages] [realtime] [verification]
|
||||
|
||||
- **Trigger:** deciding a poll/backoff cadence (sync, presence, reconciliation) from a timeout firing with nothing received, or from a fire-and-forget send that "asked" every peer.
|
||||
- **Rule:** model the round — who was actually reached, who replied, what they reported — and let only a fully answered round with nothing outstanding buy the slow cadence; re-arm the timer from the verdict of the round that just closed, never from the previous one.
|
||||
- **Why:** `messages-sync.effects.ts` set `lastSyncClean = true` inside `syncTimeout$`, so a round nobody answered dropped the poll from 10s to 15min; `sendToPeer` also returned `void` and only logged when the channel was closed, so peers listed in `getConnectedPeers()` (filled at `connectionState === 'connected'`, before the data channel opens) counted as asked. On top of that, `repeat({ delay })` read the flag at emission time, so a round that discovered missing ids was already committed to a 15-minute wait.
|
||||
- **Example:** `message-sync-round.rules.ts` (`createInventoryRound` / `recordInventoryReply` / `isInventoryRoundClean`) plus `messages-sync.effects.spec.ts`, which advances fake timers and asserts the fast cadence survives silence, a partial answer, an undelivered request, and a late reply reporting missing ids.
|
||||
|
||||
### Derive a conversation id from canonical humans, never from the ids on the wire [direct-message] [identity]
|
||||
|
||||
- **Trigger:** building or trusting a composite id (DM thread, call id, dedupe key) made of participant ids that arrived in a payload or came from a roster entry.
|
||||
- **Rule:** resolve every id through an alias index first (`buildDirectParticipantAliasIndex` → `getCanonicalDirectConversationId` / `canonicalizeDirectConversationId`), and collapse already-stored alias copies on first touch instead of only fixing new ones.
|
||||
- **Why:** `getDirectConversationId` sorted the raw pair, so a peer who addressed the local user by a provisioned foreign actor id produced a second thread; the recipient saw two conversations for one human and clicking the peer opened the empty one. Matching aliases for *admission* was already in place, which made the fork look like a delivery bug instead of an id bug.
|
||||
- **Example:** `e2e/tests/chat/cross-signal-dm-identity.spec.ts` fails with `element(s) not found` for the peer's message the moment the self-alias group is dropped from `DirectMessageService.participantAliasIndex()`.
|
||||
|
||||
### Report whether a call event was delivered before showing a live call [direct-call] [verification]
|
||||
|
||||
- **Trigger:** calling a fire-and-forget send (`sendCallEvent`, broadcast, notify) and then moving the UI into the success state.
|
||||
- **Rule:** return the transport result, ring before joining local media, and surface "reached nobody" through the same error signal the view already renders.
|
||||
- **Why:** `startCall` joined voice first and dropped the boolean from `PeerDeliveryService.sendCallEvent`, so a call to an unreachable peer showed the caller in a live-looking session that would never connect.
|
||||
- **Example:** `DirectCallService.ringParticipants` sets `deliveryError` (`call.errors.ringUndelivered`) and `private-call.component.ts` folds it into `callErrorMessage`; the e2e drives it with `window.simulateOffline()` on the caller.
|
||||
|
||||
### Never spend a retry budget on attempts the transport cannot deliver [realtime] [recovery]
|
||||
|
||||
- **Trigger:** writing or reviewing a bounded retry loop (peer reconnect, resync, delivery) that counts attempts before checking whether the channel it needs is even available.
|
||||
- **Rule:** check the dependency first and defer without counting; spend an attempt only when it can actually reach the far side, and when the budget really does run out publish a state the UI can show and re-arm the loop when the dependency returns.
|
||||
- **Why:** `peer-recovery.ts` incremented `reconnectAttempts` before `isSignalingConnected()`, so a ~60s signal outage burned all 12 attempts doing nothing, then cleared the timer and deleted the tracker entry with no user-visible state and no re-arm — the peer stayed dead until an unrelated roster event happened to heal it.
|
||||
- **Example:** `schedulePeerReconnect` now defers while signaling is down, emits `peerRecoveryStatus$` `{ status: 'failed' }` at exhaustion, and `resumeStalledPeerRecovery()` re-arms from `handleSignalingConnectionStatus`.
|
||||
|
||||
### Repair a dead data channel on the live connection before rebuilding the peer [realtime] [webrtc] [recovery]
|
||||
|
||||
- **Trigger:** handling a closed/failed `RTCDataChannel` by tracking the peer as disconnected and rebuilding the whole `RTCPeerConnection`.
|
||||
- **Rule:** while the connection is still `connected`, have the deterministically elected initiator create a replacement channel on that same connection (no renegotiation needed — the SCTP transport is already up) and let the other side adopt the incoming channel; rebuild only as the fallback when the replacement never opens.
|
||||
- **Why:** the control channel dying took voice, camera, and screen share down with it, and `replaceDataChannel` was already implemented and wired but never called — the spec asserted `not.toHaveBeenCalled()` and the README described the soft replacement as if it shipped.
|
||||
- **Example:** `e2e/tests/voice/recovery-preserves-media.spec.ts` asserts the created-`RTCPeerConnection` count stays at 1 per peer after `closeOpenDataChannels`; forcing the rebuild path makes it fail.
|
||||
|
||||
### Compare peer ids only within one signal server's identity space [realtime] [identity] [webrtc]
|
||||
|
||||
- **Trigger:** about to compare a remote `peerId` / roster `oderId` against a local id — deterministic initiator election, offer-collision politeness, reconnect election, self-filtering, or the `oderId` stamped into a voice/camera/screen payload.
|
||||
- **Rule:** resolve the local id for that peer's signal server (`getLocalOderIdForSignalUrl` where the `signalUrl` is in hand, `getIdentifyCredentialsForPeer` inside the peer manager) and elect roles only through `peer-role.rules.ts`; never reach for the home credential.
|
||||
- **Why:** one human has a different actor id per signal server, so a home-vs-foreign comparison is not antisymmetric — both peers offer (glare) or neither does until the 5s takeover, which is the "some users can't hear each other" report. It also makes your own foreign roster entry fail the self-check, so the client tries to peer with itself.
|
||||
- **Example:** `realtime-session.service.ts` wired `getLocalOderId` to `getIdentifyCredentials()` (always home) while `shouldInitiatePeer` compared it against foreign roster ids.
|
||||
|
||||
### Reproduce initiator/glare bugs with a simultaneous reconnect, not staggered joins [testing] [realtime] [webrtc]
|
||||
|
||||
- **Trigger:** writing an e2e for peer election, glare, or "cannot hear each other" and joining clients one after another.
|
||||
- **Rule:** get every client onto the roster, then reload/reconnect them with `Promise.all` so all pairs elect from the same snapshot, and assert real audio flow plus exactly one initiator per pair.
|
||||
- **Why:** staggered joins let one side's 1s fallback-offer timer serialize negotiation, so a wrong comparison still converges and the test passes on broken code — three sequential-join runs passed against the known-bad wiring before the simultaneous reconnect made it fail on audio.
|
||||
- **Example:** `e2e/tests/voice/cross-signal-initiator-election.spec.ts` — 4 users, 2 home signal servers, one shared voice channel, `Promise.all(reload)`.
|
||||
|
||||
### Interview before coding; don’t guess the fix [workflow] [bugs] [tokens]
|
||||
|
||||
- **Trigger:** about to edit product code for a bug/feature after reading the ask or Obsidian note, while acceptance, approach, or scope is still ambiguous or has real alternatives.
|
||||
@@ -88,13 +242,6 @@ Durable rules for AI agents working on this project.
|
||||
- **Why:** inbound and outbound identity bugs are independent; fixing admission on the callee does not help if the ring never leaves the caller or hits the wrong `targetUserId` on the wire.
|
||||
- **Example:** `PeerDeliveryService.sendViaSignaling` + `DirectCallService.resolveRoutableRecipientId`; e2e `e2e/tests/voice/dm-header-call-ring.spec.ts` (callee-home room, people-search call).
|
||||
|
||||
### Canonicalize direct conversation ids across cross-signal actor aliases [direct-message] [identity]
|
||||
|
||||
- **Trigger:** cross-signal DMs delivered to the callee but the caller never saw replies — User1 had two DM rows for User2, replies landed in the actor-id thread, and DM routes could throw "Cannot use direct messages without a current user" when `openConversation` ran before hydration.
|
||||
- **Rule:** incoming DM traffic must resolve to the existing home-id conversation for the same peer (`direct-message-conversation-identity.rules.ts`), merge duplicate alias threads on load/receive, queue inbound events until `currentUser` hydrates, and guard route-driven `openConversation` calls when no owner id is available yet.
|
||||
- **Why:** recipient-alias admission alone is insufficient — if `message.conversationId` still carries a foreign actor id, the client forks a second local thread and the original chat stays empty.
|
||||
- **Example:** `resolveDirectConversationId` + `mergeAliasDirectConversations` in `DirectMessageService.handleIncomingMessage`; `appendCrossSignalActorAliases` + `pickDeliveryTargetIds` when a foreign-home recipient is addressed by home id but only their shared-room actor id is routed (debug pattern: `c84813bb…` home id vs `e79a95e5…` actor on `signal.toju.app`).
|
||||
|
||||
### Decide attachment receive admission once at request time; never re-gate size in the chunk handler [attachments]
|
||||
|
||||
- **Trigger:** "Sending files between users doesn't really work" — a browser user clicked Request on a 10–50 MB generic file, the request gate (`canReceiveAttachment`) admitted it for in-memory receive, the sender streamed chunks, but `handleFileChunk` still had a leftover hard `size > MAX_AUTO_SAVE_SIZE_BYTES` rejection on the in-memory path, so every chunk was dropped, no ack was ever sent, the sender's `waitForAck` timed out, and the GUI never changed.
|
||||
|
||||
Reference in New Issue
Block a user