diff --git a/AGENTS.md b/AGENTS.md index 8b0ce52..f5d2270 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -523,8 +523,23 @@ session with **no other memory of this project**. So: for an interactive signer** (parallel kept for silent ones); the NIP-17 crew Chat therefore costs **2 approvals per member** (encrypt + sign). Steering Amber users to the **MLS Private chat** (≈1 approval per action) is the real - cure. (Not yet done: a persisted decrypt cache, so - the first load per app start still decrypts everything.) + cure. + **Crew-chat audit + fixes (2026-10-13, see `CREW_CHAT_AUDIT.md`):** ① a sent + crew message **didn't appear** — `useSendDirectMessage` never invalidated + `['group-messages', gid, …]` (the crew thread key); it now **optimistically + inserts** the sender's own plaintext (no refetch ⇒ no Amber pops). ② **All four + DM readers shared no work** — each decrypted every wrap (2 ops) under its own + query key, so every view re-decrypted the whole inbox; now a module-level + **`decryptGiftWrap` cache** (keyed `viewerPk:wrapId`, coalesced, bounded 4000, + **successes only** so a transient failure retries) is used by + `useConversations`/`useConversation`/`useGroupConversations`/`useGroupConversation`. + ③ threads dedup by rumor id; ④ `CrewChat` guards double-send with a ref; + ⑤ the Cordn path optimistically inserts too and invalidates only for silent + signers. ⚠️ **Open:** a crew with a `coordinator` tag silently uses the + sealed-relay Cordn transport instead of NIP-17 (separate history — a product + decision, not a bug); the 200-wrap read window can hide older messages. + (Still not done: persisting the decrypt cache across app restarts, so the first + load per launch still decrypts everything.) - **Sasquatch AI guide** (`src/components/SasquatchGuide.tsx`): the chat queries public relays **directly** (`useCVMClient.ts`, 3 fixed relays) and does **not** use the ContextVM server. AI goes through our own **server-side diff --git a/CREW_CHAT_AUDIT.md b/CREW_CHAT_AUDIT.md new file mode 100644 index 0000000..739d70f --- /dev/null +++ b/CREW_CHAT_AUDIT.md @@ -0,0 +1,27 @@ +# Crew chat — audit (2026-10-13) + +Scope: the crew page's **Chat** tab (`CrewChat.tsx`) across both transports — the +built-in **NIP-17** group chat and the **Cordn sealed-relay** path (`useCordnChat`) +— plus the shared DM readers it depends on (`useDirectMessages.ts`). + +## Findings + +| # | Sev | Status | Finding | +|---|---|---|---| +| 1 | High | **Fixed** | **A sent crew message didn't appear.** `useSendDirectMessage` invalidated `direct-messages` / `conversations` / `group-conversations`, but the crew thread reads `['group-messages', gid, …]` — never invalidated. For a silent signer it showed up on the next 60 s poll; for **Amber (no polling, session-cached) it never did**. Fixed with an **optimistic cache insert** of the sender's own plaintext (no refetch → no extra signer pop-ups). | +| 2 | High | **Fixed (1.6.4)** | **Amber "didn't respond" on send.** The per-recipient gift wraps (and the sealed-relay roster seal) were created with `Promise.all` — concurrent `nostrsigner:` intents collide, one is answered and the rest hit the 60 s native timeout. Now sequential for an interactive signer. | +| 3 | High | **Fixed** | **Every chat view re-decrypted the whole inbox.** Each reader (`useConversations`, `useConversation`, `useGroupConversations`, `useGroupConversation`) decrypted *every* wrap — 2 signer ops each — and each has its own query key, so opening Messages decrypted everything, then opening one crew thread decrypted it **all again**, and again per thread. Added a **shared, coalesced, bounded session decrypt cache** (`decryptGiftWrap`) keyed `viewerPk:wrapId`; only successes are cached, so a transient failure retries. | +| 4 | Med | **Fixed** | No dedup by rumor id in a thread — a re-sent rumor (same id, two wraps) could render twice. Threads now dedup by id before sorting. | +| 5 | Med | **Fixed** | **Double-send**: pressing Enter twice before React re-rendered `isPending` sent twice. Added an in-flight ref guard. | +| 6 | Med | **Fixed** | The **Cordn** path invalidated its query on every send → an Amber refetch storm. Now optimistically inserts the sent message and only invalidates for silent signers. | +| 7 | Med | Open | **Transport switches silently.** A crew with a `coordinator` tag uses the **sealed-relay Cordn** path; without one it uses **NIP-17**. They have separate histories, so a crew that later gains a coordinator appears to "lose" its chat. New crews now get the SkateSpots coordinator by default, so most new crews take the Cordn path. Needs a product decision: label it clearly, or standardise on one. | +| 8 | Med | Open | **Read limit 200.** Threads fetch the N most recent wraps addressed to the viewer and filter client-side (the wrap hides its participants, so the relay can't filter). A busy inbox can push older crew messages out of the window → the chat looks empty. Could raise the limit for group threads. | +| 9 | Low | Open | No images/attachments in the crew **Chat** (text only). The MLS **Private chat** got images; the NIP-17 path didn't. | +| 10 | Info | — | With the cache, an Amber read is now **one decrypt per wrap per session** instead of one per view. Sends still cost 2 approvals per member (NIP-17) vs ~1 per message on the MLS path. | + +## Net effect +- Sending now shows your own message immediately. +- Opening Messages ⇄ a crew thread ⇄ another crew thread no longer re-prompts. +- Amber stops timing out mid-send. + +Not click-tested in a real browser this session; `npm test` + `check-i18n` green. diff --git a/src/components/CrewChat.tsx b/src/components/CrewChat.tsx index 5aff88c..844c1bd 100644 --- a/src/components/CrewChat.tsx +++ b/src/components/CrewChat.tsx @@ -266,8 +266,12 @@ export function CrewChat({ const rosterSize = Math.max(participants.length, 1); const approvalCount = usingCordn ? rosterSize + 1 : rosterSize * 2; + const sendingRef = useRef(false); const handleSend = async (e: React.FormEvent) => { e.preventDefault(); + // Guard against a second Enter/click before React re-renders `isPending` + // (which would send twice). + if (sendingRef.current) return; const text = draft.trim(); if (!text || !user) return; @@ -280,24 +284,9 @@ export function CrewChat({ return; } - // Cordn transport: seal to the roster and route via the coordinator. - if (usingCordn) { - try { - await cordn.send(text); - setDraft(''); - } catch (err) { - toast({ - title: t('socialUi.chat.messageFailed'), - description: err instanceof Error ? err.message : t('socialUi.chat.couldNotSend'), - variant: 'destructive', - }); - } - return; - } - - // NIP-17 fallback: recipients = everyone except yourself (hook wraps self too). + // NIP-17 recipients = everyone except yourself (the hook wraps the self copy). const recipients = participants.filter((p) => p !== user.pubkey); - if (recipients.length === 0) { + if (!usingCordn && recipients.length === 0) { toast({ title: t('socialUi.chat.noOneToMessage'), description: t('socialUi.chat.noOneToMessageDesc'), @@ -305,12 +294,17 @@ export function CrewChat({ return; } + sendingRef.current = true; try { - await sendNip17({ - recipients, - content: text, - subject: t('socialUi.chat.subject', { crew: crewName }), - }); + if (usingCordn) { + await cordn.send(text); + } else { + await sendNip17({ + recipients, + content: text, + subject: t('socialUi.chat.subject', { crew: crewName }), + }); + } setDraft(''); } catch (err) { toast({ @@ -318,6 +312,8 @@ export function CrewChat({ description: err instanceof Error ? err.message : t('socialUi.chat.couldNotSend'), variant: 'destructive', }); + } finally { + sendingRef.current = false; } }; diff --git a/src/hooks/useCordnChat.ts b/src/hooks/useCordnChat.ts index 89a3950..13aabff 100644 --- a/src/hooks/useCordnChat.ts +++ b/src/hooks/useCordnChat.ts @@ -68,9 +68,9 @@ export function useCordnChat({ endpoint, roster, enabled }: UseCordnChatParams) }); const send = useMutation({ - mutationFn: async (text: string) => { + mutationFn: async (text: string): Promise => { const body = text.trim(); - if (!body) return; + if (!body) return null; if (!endpoint || !user) throw new Error('Crew chat is not available.'); if (!canEncrypt) throw new Error('Your signer does not support NIP-44 encryption.'); @@ -83,11 +83,35 @@ export function useCordnChat({ endpoint, roster, enabled }: UseCordnChatParams) const relay = nostr.relay(endpoint.relayUrl) as unknown as CordnRelay; await publishEnvelope(relay, envelope, AbortSignal.timeout(15000)); + + // Show the sender's own message immediately (see onSuccess). + return { + id: envelope.id, + pubkey: user.pubkey, + content: body, + createdAt: envelope.created_at, + cursor: Date.now(), + }; }, - onSuccess: () => { - queryClient.invalidateQueries({ - queryKey: ['cordn-chat', endpoint?.relayUrl, endpoint?.groupId, user?.pubkey], - }); + onSuccess: (optimistic) => { + // Seed our own message into the cache so it appears without a refetch — + // a refetch would re-decrypt the whole backlog (a pop-up storm on Amber). + if (optimistic) { + queryClient.setQueryData( + ['cordn-chat', endpoint?.relayUrl, endpoint?.groupId, user?.pubkey], + (old) => { + const list = old ?? []; + if (list.some((m) => m.id === optimistic.id)) return list; + return [...list, optimistic].sort((a, b) => a.cursor - b.cursor); + }, + ); + } + // Silent signers can afford the refetch; interactive ones must not. + if (!interactive) { + queryClient.invalidateQueries({ + queryKey: ['cordn-chat', endpoint?.relayUrl, endpoint?.groupId, user?.pubkey], + }); + } }, }); diff --git a/src/hooks/useDirectMessages.ts b/src/hooks/useDirectMessages.ts index bf2d46b..63171ff 100644 --- a/src/hooks/useDirectMessages.ts +++ b/src/hooks/useDirectMessages.ts @@ -219,6 +219,98 @@ function createRumor( } as NostrEvent; } +/** A decrypted NIP-17 message (the fields every reader needs). */ +interface DecryptedDm { + id: string; + pubkey: string; + content: string; + createdAt: number; + /** The rumor's `p` tags (raw recipients, sender NOT necessarily included). */ + recipients: string[]; + /** Sender + recipients, deduped (the group identity). */ + participants: string[]; + subject?: string; + replyTo?: string; +} + +/** + * Session cache of decrypted gift wraps, keyed by `viewerPk:wrapId`. + * + * Every reader (`useConversations`, `useConversation`, `useGroupConversations`, + * `useGroupConversation`) decrypts the SAME wraps — two signer operations each. + * Without a shared cache, opening Messages decrypts the whole inbox, then opening + * a single crew thread decrypts it ALL AGAIN (different query key), and each + * further thread again — a pop-up storm for an interactive signer (Amber) and + * wasted work for everyone. Decryption is deterministic and per-recipient, so a + * successful result is safe to reuse (in-flight calls are coalesced too). + * Only SUCCESSES are cached: a transient failure (e.g. an Amber timeout) is + * retried rather than remembered. Bounded to avoid unbounded growth. + */ +const DM_DECRYPT_CACHE_MAX = 4000; +const dmDecryptCache = new Map(); +const dmDecryptInFlight = new Map>(); + +/** The signer surface DM reading needs (NIP-44 decrypt). */ +interface DmSigner { + nip44?: { + decrypt: (pubkey: string, ciphertext: string) => Promise; + }; +} + +async function decryptGiftWrap( + viewerPk: string, + signer: DmSigner, + wrap: NostrEvent, +): Promise { + const key = `${viewerPk}:${wrap.id}`; + const cached = dmDecryptCache.get(key); + if (cached) return cached; + const flying = dmDecryptInFlight.get(key); + if (flying) return flying; + + const task = (async (): Promise => { + try { + const sealJson = await signer.nip44!.decrypt(wrap.pubkey, wrap.content); + const seal = JSON.parse(sealJson) as NostrEvent; + if (seal.kind !== SEAL_KIND) return null; + // SECURITY: verify the seal's signature (sender impersonation). + if (!verifyEvent(seal)) return null; + + const rumorJson = await signer.nip44!.decrypt(seal.pubkey, seal.content); + const rumor = JSON.parse(rumorJson) as NostrEvent; + if (rumor.pubkey !== seal.pubkey) return null; + if (rumor.kind !== DM_KIND) return null; + + const pTags = rumor.tags.filter((t) => t[0] === 'p').map((t) => t[1]); + const dm: DecryptedDm = { + id: rumor.id, + pubkey: rumor.pubkey, + content: rumor.content, + createdAt: rumor.created_at, + recipients: pTags, + participants: [...new Set([rumor.pubkey, ...pTags])], + subject: rumor.tags.find((t) => t[0] === 'subject')?.[1], + replyTo: rumor.tags.find((t) => t[0] === 'e')?.[1], + }; + if (dmDecryptCache.size >= DM_DECRYPT_CACHE_MAX) { + // Drop the oldest insertion (Map preserves order). + const oldest = dmDecryptCache.keys().next().value; + if (oldest !== undefined) dmDecryptCache.delete(oldest); + } + dmDecryptCache.set(key, dm); + return dm; + } catch { + // Not cached — a transient failure may succeed on the next attempt. + return null; + } finally { + dmDecryptInFlight.delete(key); + } + })(); + + dmDecryptInFlight.set(key, task); + return task; +} + /** * Hook to send NIP-17 direct messages (1:1 or group) */ @@ -406,6 +498,34 @@ export function useSendDirectMessage() { }); } + // Show the sender's own message immediately. The crew chat reads via + // `['group-messages', groupId, …]` (`useGroupConversation`) — which the list + // invalidations below do NOT cover — so a sent group message used to be + // invisible until a poll/remount, and for an interactive signer (no polling) + // never. Seed the cache with the plaintext we already hold (no refetch, so + // no extra signer pop-ups). A later natural refetch replaces it. + if (isGroup) { + const groupId = getGroupId(allTargets); + const optimistic: DirectMessage = { + id: rumor.id, + pubkey: user.pubkey, + content, + createdAt: rumor.created_at, + conversationWith: groupId, + participants: allTargets, + subject: options?.subject, + replyTo: options?.replyTo, + }; + queryClient.setQueriesData( + { queryKey: ['group-messages', groupId, user.pubkey] }, + (old) => { + const list = old ?? []; + if (list.some((m) => m.id === optimistic.id)) return list; + return [...list, optimistic].sort((a, b) => a.createdAt - b.createdAt); + }, + ); + } + // Invalidate queries to refresh message list queryClient.invalidateQueries({ queryKey: ['direct-messages'] }); queryClient.invalidateQueries({ queryKey: ['conversations'] }); @@ -508,52 +628,31 @@ export function useConversation(otherPubkey: string) { for (const giftWrap of uniqueEvents) { try { - // Decrypt gift wrap to get seal - const sealJson = await user.signer.nip44.decrypt(giftWrap.pubkey, giftWrap.content); - const seal = JSON.parse(sealJson) as NostrEvent; + // Decrypt via the shared session cache (coalesced; one decrypt per wrap + // per session even across different views). + const dm = await decryptGiftWrap(user.pubkey, user.signer, giftWrap); + if (!dm) continue; - // Verify seal is kind 13 - if (seal.kind !== SEAL_KIND) continue; - // SECURITY: the seal (kind 13) is the ONLY authentication for a NIP-17 - // message — the rumor (kind 14) is intentionally unsigned. Verify the - // seal's signature so a crafted seal can't impersonate another sender. - if (!verifyEvent(seal)) continue; - - // Decrypt seal to get rumor - const rumorJson = await user.signer.nip44.decrypt(seal.pubkey, seal.content); - const rumor = JSON.parse(rumorJson) as NostrEvent; - - // Verify rumor pubkey matches seal pubkey (prevent impersonation) - if (rumor.pubkey !== seal.pubkey) continue; - - // Verify this is a kind 14 DM - if (rumor.kind !== DM_KIND) continue; - - // Check if this message is part of the conversation with otherPubkey - const pTags = rumor.tags.filter(t => t[0] === 'p').map(t => t[1]); - - // 1:1 only. A rumor with more than one recipient is a GROUP message + // 1:1 only. A message with more than one recipient is a GROUP message // (handled by useGroupConversation). Without this guard a group chat // leaks into the 1:1 thread with each of its participants. - if (pTags.length > 1) continue; + if (dm.recipients.length > 1) continue; - const isFromOther = rumor.pubkey === otherPubkey; - const isToOther = pTags.includes(otherPubkey); - const isFromMe = rumor.pubkey === user.pubkey; - const isToMe = pTags.includes(user.pubkey); + const isFromOther = dm.pubkey === otherPubkey; + const isToOther = dm.recipients.includes(otherPubkey); + const isFromMe = dm.pubkey === user.pubkey; + const isToMe = dm.recipients.includes(user.pubkey); // Message is part of this conversation if: // - It's from the other person to me, OR // - It's from me to the other person if ((isFromOther && isToMe) || (isFromMe && isToOther)) { - const replyTo = rumor.tags.find(t => t[0] === 'e')?.[1]; - messages.push({ - id: rumor.id, - pubkey: rumor.pubkey, - content: rumor.content, - createdAt: rumor.created_at, - replyTo, + id: dm.id, + pubkey: dm.pubkey, + content: dm.content, + createdAt: dm.createdAt, + replyTo: dm.replyTo, conversationWith: otherPubkey, }); } @@ -618,47 +717,33 @@ export function useConversations() { for (const giftWrap of uniqueEvents) { try { - // Decrypt gift wrap - const sealJson = await user.signer.nip44.decrypt(giftWrap.pubkey, giftWrap.content); - const seal = JSON.parse(sealJson) as NostrEvent; + // Decrypt via the shared session cache. + const dm = await decryptGiftWrap(user.pubkey, user.signer, giftWrap); + if (!dm) continue; - if (seal.kind !== SEAL_KIND) continue; - // SECURITY: verify the seal's signature (see note above). - if (!verifyEvent(seal)) continue; - - // Decrypt seal - const rumorJson = await user.signer.nip44.decrypt(seal.pubkey, seal.content); - const rumor = JSON.parse(rumorJson) as NostrEvent; - - if (rumor.pubkey !== seal.pubkey) continue; - if (rumor.kind !== DM_KIND) continue; - - // Determine conversation partner - const pTags = rumor.tags.filter(t => t[0] === 'p').map(t => t[1]); - - // 1:1 list only. A rumor addressed to more than one recipient is a - // GROUP message, which this inbox does not surface. Without this - // guard each group would masquerade as a phantom 1:1 with whichever - // participant happened to speak, and would appear twice in the UI. - if (pTags.length > 1) continue; + // 1:1 list only. A message with more than one recipient is a GROUP + // message, which this inbox does not surface. Without this guard each + // group would masquerade as a phantom 1:1 with whichever participant + // happened to speak, and would appear twice in the UI. + if (dm.recipients.length > 1) continue; let conversationWith: string; - if (rumor.pubkey === user.pubkey) { + if (dm.pubkey === user.pubkey) { // Message from me - conversation is with the recipient - conversationWith = pTags.find(p => p !== user.pubkey) || pTags[0]; + conversationWith = dm.recipients.find((p) => p !== user.pubkey) || dm.recipients[0]; } else { // Message to me - conversation is with the sender - conversationWith = rumor.pubkey; + conversationWith = dm.pubkey; } if (!conversationWith) continue; const message: DirectMessage = { - id: rumor.id, - pubkey: rumor.pubkey, - content: rumor.content, - createdAt: rumor.created_at, + id: dm.id, + pubkey: dm.pubkey, + content: dm.content, + createdAt: dm.createdAt, conversationWith, }; @@ -742,31 +827,21 @@ export function useGroupConversations() { for (const giftWrap of uniqueEvents) { try { - const sealJson = await user.signer.nip44.decrypt(giftWrap.pubkey, giftWrap.content); - const seal = JSON.parse(sealJson) as NostrEvent; - if (seal.kind !== SEAL_KIND) continue; - if (!verifyEvent(seal)) continue; - - const rumorJson = await user.signer.nip44.decrypt(seal.pubkey, seal.content); - const rumor = JSON.parse(rumorJson) as NostrEvent; - if (rumor.pubkey !== seal.pubkey) continue; - if (rumor.kind !== DM_KIND) continue; - - const pTags = rumor.tags.filter(t => t[0] === 'p').map(t => t[1]); + const dm = await decryptGiftWrap(user.pubkey, user.signer, giftWrap); + if (!dm) continue; // 1:1 messages are surfaced by useConversations, not here. - if (pTags.length <= 1) continue; + if (dm.recipients.length <= 1) continue; - const participants = Array.from(new Set([rumor.pubkey, ...pTags])); - const groupId = getGroupId(participants); + const groupId = getGroupId(dm.participants); const message: DirectMessage = { - id: rumor.id, - pubkey: rumor.pubkey, - content: rumor.content, - createdAt: rumor.created_at, + id: dm.id, + pubkey: dm.pubkey, + content: dm.content, + createdAt: dm.createdAt, conversationWith: groupId, - participants, - subject: rumor.tags.find(t => t[0] === 'subject')?.[1], + participants: dm.participants, + subject: dm.subject, }; const existing = byGroup.get(groupId) || []; @@ -861,47 +936,31 @@ export function useGroupConversation(participants: string[]) { for (const giftWrap of uniqueEvents) { try { - const sealJson = await user.signer.nip44.decrypt(giftWrap.pubkey, giftWrap.content); - const seal = JSON.parse(sealJson) as NostrEvent; + const dm = await decryptGiftWrap(user.pubkey, user.signer, giftWrap); + if (!dm) continue; - if (seal.kind !== SEAL_KIND) continue; - if (!verifyEvent(seal)) continue; - - const rumorJson = await user.signer.nip44.decrypt(seal.pubkey, seal.content); - const rumor = JSON.parse(rumorJson) as NostrEvent; - - if (rumor.pubkey !== seal.pubkey) continue; - if (rumor.kind !== DM_KIND) continue; - - // Get all participants - const pTags = rumor.tags.filter(t => t[0] === 'p').map(t => t[1]); - const allParticipants = [...new Set([rumor.pubkey, ...pTags])]; - const messageGroupId = getGroupId(allParticipants); - - // Only include if this message is part of the target group - if (messageGroupId !== groupId) continue; - - const subject = rumor.tags.find(t => t[0] === 'subject')?.[1]; - const replyTo = rumor.tags.find(t => t[0] === 'e')?.[1]; + // Only include if this message is part of the target group. + if (getGroupId(dm.participants) !== groupId) continue; messages.push({ - id: rumor.id, - pubkey: rumor.pubkey, - content: rumor.content, - createdAt: rumor.created_at, + id: dm.id, + pubkey: dm.pubkey, + content: dm.content, + createdAt: dm.createdAt, conversationWith: groupId, - participants: allParticipants, - subject, - replyTo, + participants: dm.participants, + subject: dm.subject, + replyTo: dm.replyTo, }); - } catch (error) { console.debug('[DM] Failed to decrypt:', error); } } - // Sort by creation time (oldest first for chat display) - return messages.sort((a, b) => a.createdAt - b.createdAt); + // Dedup (a re-sent rumor can arrive in more than one wrap), then sort by + // creation time (oldest first for chat display). + const byId = new Map(messages.map((m) => [m.id, m])); + return [...byId.values()].sort((a, b) => a.createdAt - b.createdAt); }, enabled: nip44Available && participants.length > 0, ...amberSafeQueryOptions(isInteractiveSigner(user, logins)),