fix: improve chat architecture robustness and error handling (#66)

* fix: improve chat architecture robustness and error handling - Fix scroll-to-message index mismatch (was searching in wrong array) - Fix subscription memory leaks by tracking and cleaning up subscriptions - Add error handling for conversation resolution with retry UI - Add error handling for send message with toast notifications - Fix array mutation bugs in NIP-53 relay handling - Add type guards for LiveActivityMetadata - Fix RelaysDropdown O(n²) performance issue - Add loading state for send button * refactor: add stronger types and optimize message sorting - Add discriminated union types for ProtocolIdentifier (GroupIdentifier, LiveActivityIdentifier, DMIdentifier, NIP05Identifier, ChannelIdentifier) - Optimize message sorting using reverse() instead of full sort (O(n) vs O(n log n)) - Add type narrowing in adapter resolveConversation methods - Remove unused Observable import from ChatViewer --------- Co-authored-by: Claude <noreply@anthropic.com>
2026-04-09 23:16:50 +02:00 · 2026-01-12 21:12:22 +01:00
parent 159d35e347
commit 20eb19bdbb
7 changed files with 326 additions and 86 deletions
--- a/src/components/ChatViewer.tsx
+++ b/src/components/ChatViewer.tsx
@@ -1,9 +1,10 @@
-import { useMemo, useState, memo, useCallback, useRef } from "react";
+import { useMemo, useState, memo, useCallback, useRef, useEffect } from "react";
 import { use$ } from "applesauce-react/hooks";
-import { from } from "rxjs";
+import { from, catchError, of, map } from "rxjs";
 import { Virtuoso, VirtuosoHandle } from "react-virtuoso";
-import { Loader2, Reply, Zap } from "lucide-react";
+import { Loader2, Reply, Zap, AlertTriangle, RefreshCw } from "lucide-react";
 import { getZapRequest } from "applesauce-common/helpers/zap";
+import { toast } from "sonner";
 import accountManager from "@/services/accounts";
 import eventStore from "@/services/event-store";
 import type {
@@ -100,6 +101,28 @@ function isDifferentDay(timestamp1: number, timestamp2: number): boolean {
  );
 }

+/**
+ * Type guard for LiveActivityMetadata
+ */
+function isLiveActivityMetadata(value: unknown): value is LiveActivityMetadata {
+  if (!value || typeof value !== "object") return false;
+  const obj = value as Record<string, unknown>;
+  return (
+    typeof obj.status === "string" &&
+    typeof obj.hostPubkey === "string" &&
+    Array.isArray(obj.hashtags) &&
+    Array.isArray(obj.relays)
+  );
+}
+
+/**
+ * Conversation resolution result - either success with conversation or error
+ */
+type ConversationResult =
+  | { status: "loading" }
+  | { status: "success"; conversation: Conversation }
+  | { status: "error"; error: string };
+
 /**
 * ComposerReplyPreview - Shows who is being replied to in the composer
 */
@@ -308,12 +331,47 @@ export function ChatViewer({
  // Get the appropriate adapter for this protocol
  const adapter = useMemo(() => getAdapter(protocol), [protocol]);

-  // Resolve conversation from identifier (async operation)
-  const conversation = use$(
-    () => from(adapter.resolveConversation(identifier)),
-    [adapter, identifier],
+  // State for retry trigger
+  const [retryCount, setRetryCount] = useState(0);
+
+  // Resolve conversation from identifier with error handling
+  const conversationResult = use$(
+    () =>
+      from(adapter.resolveConversation(identifier)).pipe(
+        map(
+          (conv): ConversationResult => ({
+            status: "success",
+            conversation: conv,
+          }),
+        ),
+        catchError((err) => {
+          console.error("[Chat] Failed to resolve conversation:", err);
+          const errorMessage =
+            err instanceof Error ? err.message : "Failed to load conversation";
+          return of<ConversationResult>({
+            status: "error",
+            error: errorMessage,
+          });
+        }),
+      ),
+    [adapter, identifier, retryCount],
  );

+  // Extract conversation from result (null while loading or on error)
+  const conversation =
+    conversationResult?.status === "success"
+      ? conversationResult.conversation
+      : null;
+
+  // Cleanup subscriptions when conversation changes or component unmounts
+  useEffect(() => {
+    return () => {
+      if (conversation) {
+        adapter.cleanup(conversation.id);
+      }
+    };
+  }, [adapter, conversation]);
+
  // Load messages for this conversation (reactive)
  const messages = use$(
    () => (conversation ? adapter.loadMessages(conversation) : undefined),
@@ -368,18 +426,33 @@ export function ChatViewer({
  // Ref to MentionEditor for programmatic submission
  const editorRef = useRef<MentionEditorHandle>(null);

-  // Handle sending messages
+  // State for send in progress (prevents double-sends)
+  const [isSending, setIsSending] = useState(false);
+
+  // Handle sending messages with error handling
  const handleSend = async (
    content: string,
    replyToId?: string,
    emojiTags?: EmojiTag[],
  ) => {
-    if (!conversation || !hasActiveAccount) return;
-    await adapter.sendMessage(conversation, content, {
-      replyTo: replyToId,
-      emojiTags,
-    });
-    setReplyTo(undefined); // Clear reply context after sending
+    if (!conversation || !hasActiveAccount || isSending) return;
+
+    setIsSending(true);
+    try {
+      await adapter.sendMessage(conversation, content, {
+        replyTo: replyToId,
+        emojiTags,
+      });
+      setReplyTo(undefined); // Clear reply context only on success
+    } catch (error) {
+      console.error("[Chat] Failed to send message:", error);
+      const errorMessage =
+        error instanceof Error ? error.message : "Failed to send message";
+      toast.error(errorMessage);
+      // Don't clear replyTo so user can retry
+    } finally {
+      setIsSending(false);
+    }
  };

  // Handle reply button click
@@ -388,10 +461,14 @@ export function ChatViewer({
  }, []);

  // Handle scroll to message (when clicking on reply preview)
+  // Must search in messagesWithMarkers since that's what Virtuoso renders
  const handleScrollToMessage = useCallback(
    (messageId: string) => {
-      if (!messages) return;
-      const index = messages.findIndex((m) => m.id === messageId);
+      if (!messagesWithMarkers) return;
+      // Find index in the rendered array (which includes day markers)
+      const index = messagesWithMarkers.findIndex(
+        (item) => item.type === "message" && item.data.id === messageId,
+      );
      if (index !== -1 && virtuosoRef.current) {
        virtuosoRef.current.scrollToIndex({
          index,
@@ -400,7 +477,7 @@ export function ChatViewer({
        });
      }
    },
-    [messages],
+    [messagesWithMarkers],
  );

  // Handle loading older messages
@@ -438,10 +515,12 @@ export function ChatViewer({
    }
  }, [conversation?.protocol, addWindow]);

-  // Get live activity metadata if this is a NIP-53 chat
-  const liveActivity = conversation?.metadata?.liveActivity as
-    | LiveActivityMetadata
-    | undefined;
+  // Get live activity metadata if this is a NIP-53 chat (with type guard)
+  const liveActivity = isLiveActivityMetadata(
+    conversation?.metadata?.liveActivity,
+  )
+    ? conversation?.metadata?.liveActivity
+    : undefined;

  // Derive participants from messages for live activities (unique pubkeys who have chatted)
  const derivedParticipants = useMemo(() => {
@@ -474,14 +553,40 @@ export function ChatViewer({
    liveActivity?.hostPubkey,
  ]);

-  if (!conversation) {
+  // Handle loading state
+  if (!conversationResult || conversationResult.status === "loading") {
    return (
-      <div className="flex h-full items-center justify-center text-muted-foreground">
-        Loading conversation...
+      <div className="flex h-full flex-col items-center justify-center gap-2 text-muted-foreground">
+        <Loader2 className="size-6 animate-spin" />
+        <span>Loading conversation...</span>
      </div>
    );
  }

+  // Handle error state with retry option
+  if (conversationResult.status === "error") {
+    return (
+      <div className="flex h-full flex-col items-center justify-center gap-3 text-muted-foreground p-4">
+        <AlertTriangle className="size-8 text-destructive" />
+        <span className="text-center text-sm">{conversationResult.error}</span>
+        <Button
+          variant="outline"
+          size="sm"
+          onClick={() => setRetryCount((c) => c + 1)}
+          className="gap-2"
+        >
+          <RefreshCw className="size-3" />
+          Retry
+        </Button>
+      </div>
+    );
+  }
+
+  // At this point conversation is guaranteed to exist
+  if (!conversation) {
+    return null; // Should never happen, but satisfies TypeScript
+  }
+
  return (
    <div className="flex h-full flex-col">
      {/* Header with conversation info and controls */}
@@ -675,11 +780,12 @@ export function ChatViewer({
              type="button"
              variant="secondary"
              className="flex-shrink-0 h-[2.5rem]"
+              disabled={isSending}
              onClick={() => {
                editorRef.current?.submit();
              }}
            >
-              Send
+              {isSending ? <Loader2 className="size-4 animate-spin" /> : "Send"}
            </Button>
          </div>
        </div>
--- a/src/components/chat/RelaysDropdown.tsx
+++ b/src/components/chat/RelaysDropdown.tsx
@@ -21,35 +21,34 @@ interface RelaysDropdownProps {
 export function RelaysDropdown({ conversation }: RelaysDropdownProps) {
  const { relays: relayStates } = useRelayState();

-  // Get relays for this conversation
-  const relays: string[] = [];
+  // Get relays for this conversation (immutable pattern)
+  const liveActivityRelays = conversation.metadata?.liveActivity?.relays;
+  const relays: string[] =
+    Array.isArray(liveActivityRelays) && liveActivityRelays.length > 0
+      ? liveActivityRelays
+      : conversation.metadata?.relayUrl
+        ? [conversation.metadata.relayUrl]
+        : [];

-  // NIP-53: Multiple relays from liveActivity
-  const liveActivityRelays = conversation.metadata?.liveActivity?.relays as
-    | string[]
-    | undefined;
-  if (liveActivityRelays?.length) {
-    relays.push(...liveActivityRelays);
-  }
-  // NIP-29: Single group relay (fallback)
-  else if (conversation.metadata?.relayUrl) {
-    relays.push(conversation.metadata.relayUrl);
-  }
-
-  // Normalize URLs for state lookup
-  const normalizedRelays = relays.map((url) => {
+  // Pre-compute normalized URLs and state lookups in a single pass (O(n))
+  const relayData = relays.map((url) => {
+    let normalizedUrl: string;
    try {
-      return normalizeRelayURL(url);
+      normalizedUrl = normalizeRelayURL(url);
    } catch {
-      return url;
+      normalizedUrl = url;
    }
+    const state = relayStates[normalizedUrl];
+    return {
+      url,
+      normalizedUrl,
+      state,
+      isConnected: state?.connectionState === "connected",
+    };
  });

  // Count connected relays
-  const connectedCount = normalizedRelays.filter((url) => {
-    const state = relayStates[url];
-    return state?.connectionState === "connected";
-  }).length;
+  const connectedCount = relayData.filter((r) => r.isConnected).length;

  if (relays.length === 0) {
    return null; // Don't show if no relays
@@ -70,9 +69,7 @@ export function RelaysDropdown({ conversation }: RelaysDropdownProps) {
          Relays ({relays.length})
        </div>
        <div className="space-y-1 p-1">
-          {relays.map((url) => {
-            const normalizedUrl = normalizedRelays[relays.indexOf(url)];
-            const state = relayStates[normalizedUrl];
+          {relayData.map(({ url, state }) => {
            const connIcon = getConnectionIcon(state);
            const authIcon = getAuthIcon(state);

--- a/src/lib/chat/adapters/base-adapter.ts
+++ b/src/lib/chat/adapters/base-adapter.ts
@@ -1,4 +1,4 @@
-import type { Observable } from "rxjs";
+import type { Observable, Subscription } from "rxjs";
 import type {
  Conversation,
  Message,
@@ -29,11 +29,38 @@ export interface SendMessageOptions {
 * - Message loading and sending
 * - Conversation management
 * - Protocol capabilities
+ *
+ * Adapters manage their own relay subscriptions. Call cleanup() when
+ * a conversation is closed to prevent memory leaks.
 */
 export abstract class ChatProtocolAdapter {
  abstract readonly protocol: ChatProtocol;
  abstract readonly type: ConversationType;

+  /** Active relay subscriptions by conversation ID */
+  protected subscriptions = new Map<string, Subscription>();
+
+  /**
+   * Cleanup subscriptions for a specific conversation
+   * Should be called when a chat window is closed
+   */
+  cleanup(conversationId: string): void {
+    const sub = this.subscriptions.get(conversationId);
+    if (sub) {
+      sub.unsubscribe();
+      this.subscriptions.delete(conversationId);
+    }
+  }
+
+  /**
+   * Cleanup all subscriptions
+   * Should be called when the adapter is no longer needed
+   */
+  cleanupAll(): void {
+    this.subscriptions.forEach((sub) => sub.unsubscribe());
+    this.subscriptions.clear();
+  }
+
  /**
   * Parse an identifier string to determine if this adapter can handle it
   * Returns null if the identifier doesn't match this protocol
--- a/src/lib/chat/adapters/nip-29-adapter.ts
+++ b/src/lib/chat/adapters/nip-29-adapter.ts
@@ -101,6 +101,12 @@ export class Nip29Adapter extends ChatProtocolAdapter {
  async resolveConversation(
    identifier: ProtocolIdentifier,
  ): Promise<Conversation> {
+    // This adapter only handles group identifiers
+    if (identifier.type !== "group") {
+      throw new Error(
+        `NIP-29 adapter cannot handle identifier type: ${identifier.type}`,
+      );
+    }
    const groupId = identifier.value;
    const relayUrl = identifier.relays?.[0];

@@ -330,8 +336,12 @@ export class Nip29Adapter extends ChatProtocolAdapter {
      filter.since = options.after;
    }

+    // Clean up any existing subscription for this conversation
+    const conversationId = `nip-29:${relayUrl}'${groupId}`;
+    this.cleanup(conversationId);
+
    // Start a persistent subscription to the group relay
-    pool
+    const subscription = pool
      .subscription([relayUrl], [filter], {
        eventStore,
      })
@@ -347,6 +357,9 @@ export class Nip29Adapter extends ChatProtocolAdapter {
        },
      });

+    // Store subscription for cleanup
+    this.subscriptions.set(conversationId, subscription);
+
    // Return observable from EventStore which will update automatically
    return eventStore.timeline(filter).pipe(
      map((events) => {
@@ -360,7 +373,10 @@ export class Nip29Adapter extends ChatProtocolAdapter {
        });

        console.log(`[NIP-29] Timeline has ${messages.length} events`);
-        return messages.sort((a, b) => a.timestamp - b.timestamp);
+        // EventStore timeline returns events sorted by created_at desc,
+        // we need ascending order for chat. Since it's already sorted,
+        // just reverse instead of full sort (O(n) vs O(n log n))
+        return messages.reverse();
      }),
    );
  }
@@ -406,7 +422,9 @@ export class Nip29Adapter extends ChatProtocolAdapter {
      return this.eventToMessage(event, conversation.id);
    });

-    return messages.sort((a, b) => a.timestamp - b.timestamp);
+    // loadMoreMessages returns events in desc order from relay,
+    // reverse for ascending chronological order
+    return messages.reverse();
  }

  /**
--- a/src/lib/chat/adapters/nip-53-adapter.ts
+++ b/src/lib/chat/adapters/nip-53-adapter.ts
@@ -84,11 +84,13 @@ export class Nip53Adapter extends ChatProtocolAdapter {
  async resolveConversation(
    identifier: ProtocolIdentifier,
  ): Promise<Conversation> {
-    const { pubkey, identifier: dTag } = identifier.value as {
-      kind: number;
-      pubkey: string;
-      identifier: string;
-    };
+    // This adapter only handles live-activity identifiers
+    if (identifier.type !== "live-activity") {
+      throw new Error(
+        `NIP-53 adapter cannot handle identifier type: ${identifier.type}`,
+      );
+    }
+    const { pubkey, identifier: dTag } = identifier.value;
    const relayHints = identifier.relays || [];

    const activePubkey = accountManager.active$.value?.pubkey;
@@ -241,10 +243,13 @@ export class Nip53Adapter extends ChatProtocolAdapter {
    const aTagValue = `30311:${pubkey}:${identifier}`;

    // Get relays from live activity metadata or fall back to relayUrl
-    const relays = liveActivity?.relays || [];
-    if (relays.length === 0 && conversation.metadata?.relayUrl) {
-      relays.push(conversation.metadata.relayUrl);
-    }
+    // Use immutable pattern to avoid mutating metadata
+    const relays =
+      liveActivity?.relays && liveActivity.relays.length > 0
+        ? liveActivity.relays
+        : conversation.metadata?.relayUrl
+          ? [conversation.metadata.relayUrl]
+          : [];

    if (relays.length === 0) {
      throw new Error("No relays available for live chat");
@@ -268,8 +273,11 @@ export class Nip53Adapter extends ChatProtocolAdapter {
      filter.since = options.after;
    }

+    // Clean up any existing subscription for this conversation
+    this.cleanup(conversation.id);
+
    // Start a persistent subscription to the relays
-    pool
+    const subscription = pool
      .subscription(relays, [filter], {
        eventStore,
      })
@@ -285,6 +293,9 @@ export class Nip53Adapter extends ChatProtocolAdapter {
        },
      });

+    // Store subscription for cleanup
+    this.subscriptions.set(conversation.id, subscription);
+
    // Return observable from EventStore which will update automatically
    return eventStore.timeline(filter).pipe(
      map((events) => {
@@ -302,7 +313,10 @@ export class Nip53Adapter extends ChatProtocolAdapter {
          .filter((msg): msg is Message => msg !== null);

        console.log(`[NIP-53] Timeline has ${messages.length} events`);
-        return messages.sort((a, b) => a.timestamp - b.timestamp);
+        // EventStore timeline returns events sorted by created_at desc,
+        // we need ascending order for chat. Since it's already sorted,
+        // just reverse instead of full sort (O(n) vs O(n log n))
+        return messages.reverse();
      }),
    );
  }
@@ -329,10 +343,13 @@ export class Nip53Adapter extends ChatProtocolAdapter {
    const aTagValue = `30311:${pubkey}:${identifier}`;

    // Get relays from live activity metadata or fall back to relayUrl
-    const relays = liveActivity?.relays || [];
-    if (relays.length === 0 && conversation.metadata?.relayUrl) {
-      relays.push(conversation.metadata.relayUrl);
-    }
+    // Use immutable pattern to avoid mutating metadata
+    const relays =
+      liveActivity?.relays && liveActivity.relays.length > 0
+        ? liveActivity.relays
+        : conversation.metadata?.relayUrl
+          ? [conversation.metadata.relayUrl]
+          : [];

    if (relays.length === 0) {
      throw new Error("No relays available for live chat");
@@ -368,7 +385,9 @@ export class Nip53Adapter extends ChatProtocolAdapter {
      })
      .filter((msg): msg is Message => msg !== null);

-    return messages.sort((a, b) => a.timestamp - b.timestamp);
+    // loadMoreMessages returns events in desc order from relay,
+    // reverse for ascending chronological order
+    return messages.reverse();
  }

  /**
@@ -400,11 +419,13 @@ export class Nip53Adapter extends ChatProtocolAdapter {
    const { pubkey, identifier } = activityAddress;
    const aTagValue = `30311:${pubkey}:${identifier}`;

-    // Get relays
-    const relays = liveActivity?.relays || [];
-    if (relays.length === 0 && conversation.metadata?.relayUrl) {
-      relays.push(conversation.metadata.relayUrl);
-    }
+    // Get relays - use immutable pattern to avoid mutating metadata
+    const relays =
+      liveActivity?.relays && liveActivity.relays.length > 0
+        ? liveActivity.relays
+        : conversation.metadata?.relayUrl
+          ? [conversation.metadata.relayUrl]
+          : [];

    if (relays.length === 0) {
      throw new Error("No relays available for sending message");
@@ -475,10 +496,14 @@ export class Nip53Adapter extends ChatProtocolAdapter {
          relays?: string[];
        }
      | undefined;
-    const relays = liveActivity?.relays || [];
-    if (relays.length === 0 && conversation.metadata?.relayUrl) {
-      relays.push(conversation.metadata.relayUrl);
-    }
+
+    // Get relays - use immutable pattern to avoid mutating metadata
+    const relays =
+      liveActivity?.relays && liveActivity.relays.length > 0
+        ? liveActivity.relays
+        : conversation.metadata?.relayUrl
+          ? [conversation.metadata.relayUrl]
+          : [];

    if (relays.length === 0) {
      console.warn("[NIP-53] No relays for loading reply message");
--- a/src/lib/chat/adapters/nip-c7-adapter.ts
+++ b/src/lib/chat/adapters/nip-c7-adapter.ts
@@ -93,8 +93,15 @@ export class NipC7Adapter extends ChatProtocolAdapter {
        throw new Error(`Failed to resolve NIP-05: ${identifier.value}`);
      }
      pubkey = resolved;
-    } else {
+    } else if (
+      identifier.type === "chat-partner" ||
+      identifier.type === "dm-recipient"
+    ) {
      pubkey = identifier.value;
+    } else {
+      throw new Error(
+        `NIP-C7 adapter cannot handle identifier type: ${identifier.type}`,
+      );
    }

    const activePubkey = accountManager.active$.value?.pubkey;
--- a/src/types/chat.ts
+++ b/src/types/chat.ts
@@ -120,14 +120,74 @@ export interface Message {
 }

 /**
- * Protocol-specific identifier
+ * NIP-29 group identifier
+ */
+export interface GroupIdentifier {
+  type: "group";
+  /** Group ID (e.g., "bitcoin-dev") */
+  value: string;
+  /** Relay URL where the group is hosted (required for NIP-29) */
+  relays: string[];
+}
+
+/**
+ * NIP-53 live activity identifier
+ */
+export interface LiveActivityIdentifier {
+  type: "live-activity";
+  /** Address pointer for the live activity */
+  value: {
+    kind: 30311;
+    pubkey: string;
+    identifier: string;
+  };
+  /** Relay hints from naddr encoding */
+  relays?: string[];
+}
+
+/**
+ * NIP-C7/NIP-17 direct message identifier (resolved pubkey)
+ */
+export interface DMIdentifier {
+  type: "dm-recipient" | "chat-partner";
+  /** Recipient pubkey (hex) */
+  value: string;
+  /** Relay hints */
+  relays?: string[];
+}
+
+/**
+ * NIP-C7 NIP-05 identifier (needs resolution)
+ */
+export interface NIP05Identifier {
+  type: "chat-partner-nip05";
+  /** NIP-05 address to resolve */
+  value: string;
+  /** Relay hints */
+  relays?: string[];
+}
+
+/**
+ * NIP-28 channel identifier (future)
+ */
+export interface ChannelIdentifier {
+  type: "channel";
+  /** Channel creation event ID or address */
+  value: string;
+  /** Relay hints */
+  relays?: string[];
+}
+
+/**
+ * Protocol-specific identifier - discriminated union
 * Returned by adapter parseIdentifier()
 */
-export interface ProtocolIdentifier {
-  type: string; // e.g., 'dm-recipient', 'channel-event', 'group-id'
-  value: any; // Protocol-specific value
-  relays?: string[]; // Relay hints from bech32 encoding
-}
+export type ProtocolIdentifier =
+  | GroupIdentifier
+  | LiveActivityIdentifier
+  | DMIdentifier
+  | NIP05Identifier
+  | ChannelIdentifier;

 /**
 * Chat command parsing result