Merge pull request #1013 from PR0M3TH3AN/codex/revise-comment-publishing-logic-and-tests

Require active signer for comment publishing

js/nostr/commentEvents.js

@@ -1023,7 +1023,6 @@ export async function publishComment(
   {
     resolveActiveSigner,
     shouldRequestExtensionPermissions,
-    signEventWithPrivateKey,
     DEFAULT_NIP07_PERMISSION_METHODS,
   } = {},
 ) {
@@ -1036,9 +1035,12 @@ export async function publishComment(
     return { ok: false, error: "invalid-target" };
   }
 
-  const actorPubkey = await client.ensureSessionActor();
+  const actorPubkey =
+    typeof client?.pubkey === "string" && client.pubkey.trim()
+      ? client.pubkey.trim()
+      : "";
   if (!actorPubkey) {
-    return { ok: false, error: "missing-actor" };
+    return { ok: false, error: "auth-required" };
   }
 
   const createdAt =
@@ -1097,71 +1099,46 @@ export async function publishComment(
 
   let signedEvent = null;
 
-  const normalizedActor =
-    typeof actorPubkey === "string" ? actorPubkey.toLowerCase() : "";
-  const normalizedLogged =
-    typeof client.pubkey === "string" ? client.pubkey.toLowerCase() : "";
-
   const resolveSignerFn =
     typeof resolveActiveSigner === "function" ? resolveActiveSigner : null;
   const signer = resolveSignerFn ? resolveSignerFn(actorPubkey) : null;
 
-  const canUseActiveSigner =
-    normalizedActor &&
-    normalizedActor === normalizedLogged &&
-    signer &&
-    typeof signer.signEvent === "function";
-
-  if (canUseActiveSigner) {
-    let permissionResult = { ok: true };
-    const shouldRequestPermissions =
-      typeof shouldRequestExtensionPermissions === "function"
-        ? shouldRequestExtensionPermissions(signer)
-        : false;
-
-    if (shouldRequestPermissions) {
-      permissionResult = await client.ensureExtensionPermissions(
-        DEFAULT_NIP07_PERMISSION_METHODS,
-      );
-    }
-
-    if (permissionResult.ok) {
-      try {
-        signedEvent = await signer.signEvent(event);
-      } catch (error) {
-        userLogger.warn(
-          "[nostr] Failed to sign comment event with active signer:",
-          error,
-        );
-        return { ok: false, error: "signing-failed", details: error };
-      }
-    } else {
-      userLogger.warn(
-        "[nostr] Active signer permissions missing; signing comment with session key.",
-        permissionResult.error,
-      );
-    }
+  if (!signer || typeof signer.signEvent !== "function") {
+    return { ok: false, error: "auth-required" };
   }
 
-  if (!signedEvent) {
-    if (typeof signEventWithPrivateKey !== "function") {
-      return { ok: false, error: "signing-unavailable" };
-    }
-    try {
-      if (!client.sessionActor || client.sessionActor.pubkey !== actorPubkey) {
-        await client.ensureSessionActor(true);
-      }
-      if (!client.sessionActor || client.sessionActor.pubkey !== actorPubkey) {
-        throw new Error("session-actor-mismatch");
-      }
-      signedEvent = signEventWithPrivateKey(
-        event,
-        client.sessionActor.privateKey,
-      );
-    } catch (error) {
-      userLogger.warn("[nostr] Failed to sign comment event with session key:", error);
-      return { ok: false, error: "signing-failed", details: error };
-    }
+  let permissionResult = { ok: true };
+  const shouldRequestPermissions =
+    typeof shouldRequestExtensionPermissions === "function"
+      ? shouldRequestExtensionPermissions(signer)
+      : false;
+
+  if (shouldRequestPermissions) {
+    permissionResult = await client.ensureExtensionPermissions(
+      DEFAULT_NIP07_PERMISSION_METHODS,
+    );
+  }
+
+  if (!permissionResult.ok) {
+    userLogger.warn(
+      "[nostr] Active signer permissions missing; comment publish requires login.",
+      permissionResult.error,
+    );
+    return {
+      ok: false,
+      error: "auth-required",
+      details: permissionResult,
+    };
+  }
+
+  try {
+    signedEvent = await signer.signEvent(event);
+  } catch (error) {
+    userLogger.warn(
+      "[nostr] Failed to sign comment event with active signer:",
+      error,
+    );
+    return { ok: false, error: "signing-failed", details: error };
   }
 
   const relayList = sanitizeRelayList(options.relays, client.relays);

js/ui/videoModalCommentController.js

@@ -628,7 +628,7 @@ export default class VideoModalCommentController {
     try {
       const result = await publishPromise;
       if (!result?.ok || !result.event) {
-        throw result?.error || new Error("publish-failed");
+        throw result || { error: "publish-failed" };
       }
 
       const event = this.enrichCommentEvent(result.event);
@@ -650,6 +650,23 @@ export default class VideoModalCommentController {
       this.applyCommentComposerAuthState();
       this.videoModal.setCommentStatus?.("Comment posted.");
     } catch (error) {
+      const authRequired =
+        (error && typeof error === "object" && error.error === "auth-required") ||
+        error === "auth-required";
+
+      if (authRequired) {
+        this.videoModal.setCommentComposerState?.({
+          disabled: true,
+          reason: "login-required",
+        });
+        this.handleVideoModalCommentLoginRequired(detail);
+        this.videoModal.setCommentComposerState?.({
+          disabled: true,
+          reason: "login-required",
+        });
+        return;
+      }
+
       devLogger.warn("[comment] Failed to publish comment:", error);
       this.videoModal.setCommentComposerState?.({
         disabled: false,

tests/nostr/comment-events.test.mjs

@@ -10,9 +10,9 @@ import {
 
 function createMockClient({
   actorPubkey = "actor-pubkey",
-  sessionPrivateKey = "session-private",
   relays = ["wss://primary"],
   publishResults = [],
+  extensionPermissions = { ok: true },
 } = {}) {
   const publishCalls = [];
   const pool = {
@@ -34,29 +34,14 @@ function createMockClient({
     sub: () => ({ on() {}, unsub() {} }),
   };
 
-  const ensureSessionActorCalls = [];
   const client = {
     pool,
     relays,
     pubkey: actorPubkey,
-    sessionActor: {
-      pubkey: actorPubkey,
-      privateKey: sessionPrivateKey,
-    },
-    ensureSessionActor: async (forceRefresh = false) => {
-      ensureSessionActorCalls.push(forceRefresh);
-      if (forceRefresh && !client.sessionActor) {
-        client.sessionActor = {
-          pubkey: actorPubkey,
-          privateKey: sessionPrivateKey,
-        };
-      }
-      return actorPubkey;
-    },
-    ensureExtensionPermissions: async () => ({ ok: true }),
+    ensureExtensionPermissions: async () => extensionPermissions,
   };
 
-  return { client, pool, publishCalls, ensureSessionActorCalls };
+  return { client, pool, publishCalls };
 }
 
 function buildCommentEventTags(event) {
@@ -83,8 +68,6 @@ test("publishComment prefers active signer when available", async () => {
     },
   };
 
-  let privateKeySignCalls = 0;
-
   const result = await publishComment(
     client,
     {
@@ -102,10 +85,6 @@ test("publishComment prefers active signer when available", async () => {
     {
       resolveActiveSigner: () => signer,
       shouldRequestExtensionPermissions: () => true,
-      signEventWithPrivateKey: () => {
-        privateKeySignCalls += 1;
-        return null;
-      },
       DEFAULT_NIP07_PERMISSION_METHODS: ["signEvent"],
     },
   );
@@ -127,7 +106,6 @@ test("publishComment prefers active signer when available", async () => {
     "publish should target the caller supplied relays",
   );
 
-  assert.equal(privateKeySignCalls, 0, "session key signing should be skipped when active signer succeeds");
   assert.equal(signerCalls.length, 1, "active signer should sign the event once");
 
   const eventTags = buildCommentEventTags(signerCalls[0]);
@@ -146,20 +124,12 @@ test("publishComment prefers active signer when available", async () => {
   );
 });
 
-test("publishComment falls back to session signer when active signer is missing", async () => {
-  const {
-    client,
-    publishCalls,
-    ensureSessionActorCalls,
-  } = createMockClient();
+test("publishComment rejects when active signer is unavailable", async () => {
+  const { client, publishCalls } = createMockClient();
 
-  client.sessionActor = null;
-
-  let privateKeySignCalls = 0;
-  const signEventWithPrivateKey = (event, key) => {
-    privateKeySignCalls += 1;
-    assert.equal(key, "session-private", "session key should be used when rehydrated");
-    return { ...event, id: "signed-by-session" };
+  client.sessionActor = {
+    pubkey: client.pubkey,
+    privateKey: "session-private",
   };
 
   const result = await publishComment(
@@ -172,28 +142,26 @@ test("publishComment falls back to session signer when active signer is missing"
     {
       resolveActiveSigner: () => null,
       shouldRequestExtensionPermissions: () => false,
-      signEventWithPrivateKey,
       DEFAULT_NIP07_PERMISSION_METHODS: [],
     },
   );
 
-  assert.equal(result.ok, true, "publish should succeed with session signer");
-  assert.equal(privateKeySignCalls, 1, "session signing should occur exactly once");
-  assert.equal(
-    ensureSessionActorCalls.includes(true),
-    true,
-    "ensureSessionActor should be invoked with force refresh",
-  );
-  assert.equal(publishCalls.length, 1, "publish should still occur once");
+  assert.equal(result.ok, false, "publish should fail without an active signer");
+  assert.equal(result.error, "auth-required", "missing signer should yield auth-required");
+  assert.equal(publishCalls.length, 0, "publish should not be attempted without a signer");
 });
 
 test("publishComment accepts legacy targets with only an event id", async () => {
   const { client, publishCalls } = createMockClient();
 
   let signCalls = 0;
-  const signEventWithPrivateKey = (event) => {
-    signCalls += 1;
-    return { ...event, id: `signed-${signCalls}` };
+  const signer = {
+    type: "extension",
+    pubkey: client.pubkey,
+    signEvent: (event) => {
+      signCalls += 1;
+      return { ...event, id: `signed-${signCalls}` };
+    },
   };
 
   const result = await publishComment(
@@ -204,15 +172,14 @@ test("publishComment accepts legacy targets with only an event id", async () =>
     },
     { content: "Legacy comment" },
     {
-      resolveActiveSigner: () => null,
+      resolveActiveSigner: () => signer,
       shouldRequestExtensionPermissions: () => false,
-      signEventWithPrivateKey,
       DEFAULT_NIP07_PERMISSION_METHODS: [],
     },
   );
 
   assert.equal(result.ok, true, "publishing should succeed without a definition address");
-  assert.equal(signCalls, 1, "session signer should be used to sign the event");
+  assert.equal(signCalls, 1, "active signer should be used to sign the event");
   assert.equal(publishCalls.length, 1, "event should be published exactly once");
 
   const publishedEvent = publishCalls[0]?.event;
@@ -249,14 +216,19 @@ test("publishComment derives root and parent metadata from parent comment tags",
     ],
   };
 
+  const signer = {
+    type: "extension",
+    pubkey: client.pubkey,
+    signEvent: (event) => ({ ...event, id: "signed" }),
+  };
+
   const result = await publishComment(
     client,
     { parentComment: parentEvent },
     { content: "Reply" },
     {
-      resolveActiveSigner: () => null,
+      resolveActiveSigner: () => signer,
       shouldRequestExtensionPermissions: () => false,
-      signEventWithPrivateKey: (event) => ({ ...event, id: "signed" }),
       DEFAULT_NIP07_PERMISSION_METHODS: [],
     },
   );
@@ -277,7 +249,11 @@ test("publishComment derives root and parent metadata from parent comment tags",
 test("publishComment emits only video event #e tag when address and parent are absent", async () => {
   const { client, publishCalls } = createMockClient();
 
-  const signEventWithPrivateKey = (event) => ({ ...event, id: "solo" });
+  const signer = {
+    type: "extension",
+    pubkey: client.pubkey,
+    signEvent: (event) => ({ ...event, id: "solo" }),
+  };
 
   const result = await publishComment(
     client,
@@ -286,9 +262,8 @@ test("publishComment emits only video event #e tag when address and parent are a
     },
     { content: "Solo" },
     {
-      resolveActiveSigner: () => null,
+      resolveActiveSigner: () => signer,
       shouldRequestExtensionPermissions: () => false,
-      signEventWithPrivateKey,
       DEFAULT_NIP07_PERMISSION_METHODS: [],
     },
   );

tests/video-modal-comments.test.mjs

@@ -331,6 +331,107 @@ test(
   },
 );
 
+test(
+  "VideoModalCommentController prompts login when publish requires authentication",
+  async () => {
+    const composerStates = [];
+    const loginLifecycle = [];
+    const errorMessages = [];
+
+    const loginModal = {
+      openModal: ({ triggerElement } = {}) => {
+        loginLifecycle.push({ type: "openModal", triggerElement });
+        return true;
+      },
+    };
+
+    const controller = new VideoModalCommentController({
+      commentThreadService: {
+        setCallbacks: () => {},
+        processIncomingEvent: () => {},
+      },
+      videoModal: {
+        setCommentComposerState: (state) => {
+          composerStates.push({ ...state });
+        },
+        resetCommentComposer: () => {
+          throw new Error("composer should not reset when auth is required");
+        },
+        setCommentStatus: () => {},
+        appendComment: () => {},
+      },
+      auth: {
+        isLoggedIn: () => true,
+        initializeLoginModalController: ({ logIfMissing }) => {
+          loginLifecycle.push({ type: "initialize", logIfMissing });
+        },
+        getLoginModalController: () => loginModal,
+        requestLogin: ({ allowAccountSelection }) => {
+          loginLifecycle.push({ type: "request", allowAccountSelection });
+          return Promise.resolve(true);
+        },
+      },
+      callbacks: {
+        showError: (message) => {
+          errorMessages.push(message);
+        },
+        showStatus: () => {},
+        muteAuthor: () => Promise.resolve(),
+        shouldHideAuthor: () => false,
+      },
+      services: {
+        publishComment: async () => ({ ok: false, error: "auth-required" }),
+      },
+      utils: {
+        normalizeHexPubkey: (value) => value?.toLowerCase?.() || value,
+      },
+    });
+
+    controller.currentVideo = {
+      id: "video-auth",
+      pubkey: "AUTHORPK",
+      enableComments: true,
+      kind: 30078,
+      tags: [],
+    };
+    controller.modalCommentState.videoEventId = "video-auth";
+    controller.modalCommentState.videoKind = "30078";
+    controller.modalCommentState.videoAuthorPubkey = "AUTHORPK";
+
+    const triggerElement = { id: "comment-trigger" };
+
+    await controller.handleVideoModalCommentSubmit({
+      text: "Needs auth",
+      triggerElement,
+    });
+
+    const lastComposerState = composerStates[composerStates.length - 1];
+    assert.deepStrictEqual(
+      lastComposerState,
+      { disabled: true, reason: "login-required" },
+      "composer should remain disabled while prompting for login",
+    );
+    assert.equal(errorMessages.length, 0, "auth-required should not surface as an error");
+
+    assert.equal(
+      loginLifecycle.some((step) => step.type === "initialize"),
+      true,
+      "login modal controller should be initialized",
+    );
+    assert.equal(
+      loginLifecycle.some((step) => step.type === "openModal"),
+      true,
+      "login modal should attempt to open",
+    );
+    const modalOpen = loginLifecycle.find((step) => step.type === "openModal");
+    assert.equal(
+      modalOpen?.triggerElement,
+      triggerElement,
+      "trigger element should propagate to login modal",
+    );
+  },
+);
+
 test(
   "VideoModalCommentController includes parent metadata when replying",
   async () => {