diff --git a/.codex/skills/make-closed-issue/SKILL.md b/.codex/skills/make-closed-issue/SKILL.md index 4fc10799..d4459111 100644 --- a/.codex/skills/make-closed-issue/SKILL.md +++ b/.codex/skills/make-closed-issue/SKILL.md @@ -71,7 +71,7 @@ gh issue create \ --title "ISSUE_TITLE" \ --body "ISSUE_DESCRIPTION" \ --label "LABEL1,LABEL2" \ - --assignee plebe1us + --assignee tomcasaburi ``` Capture the issue number from the output. diff --git a/.cursor/skills/make-closed-issue/SKILL.md b/.cursor/skills/make-closed-issue/SKILL.md index 4fc10799..d4459111 100644 --- a/.cursor/skills/make-closed-issue/SKILL.md +++ b/.cursor/skills/make-closed-issue/SKILL.md @@ -71,7 +71,7 @@ gh issue create \ --title "ISSUE_TITLE" \ --body "ISSUE_DESCRIPTION" \ --label "LABEL1,LABEL2" \ - --assignee plebe1us + --assignee tomcasaburi ``` Capture the issue number from the output. diff --git a/README.md b/README.md index 1d11b543..245abefc 100644 --- a/README.md +++ b/README.md @@ -897,6 +897,7 @@ for (const accountComment of accountComments) { // it is recommended to show a label in the UI if accountComment.state is 'pending' or 'failed' console.log("comment", accountComment.index, "is status", accountComment.state); } +// `state` becomes `failed` as soon as a pending local publish records terminal failure (`publishingState === "failed"` and `state === "stopped"`) or a publish error, instead of waiting for the 20-minute fallback. // note: accountComment.index can change after deletions; prefer commentCid for stable identifiers // all my own votes diff --git a/src/hooks/accounts/accounts.test.ts b/src/hooks/accounts/accounts.test.ts index fa807165..70adb2ad 100644 --- a/src/hooks/accounts/accounts.test.ts +++ b/src/hooks/accounts/accounts.test.ts @@ -1558,6 +1558,63 @@ describe("accounts", () => { await waitForErr(() => renderedErr.result.current.error !== undefined); expect(renderedErr.result.current.error?.message).toBe("publish failed"); expect(renderedErr.result.current.errors).toHaveLength(1); + expect(renderedErr.result.current.state).toBe("failed"); + }); + + test("useAccountComment keeps pending when publishingState is failed but publication state is not stopped", async () => { + const state = accountsStore.getState() as any; + const accountId = state.activeAccountId || Object.keys(state.accounts)[0]; + const existing = state.accountsComments?.[accountId] || []; + const failedIndex = existing.length; + accountsStore.setState((s: any) => ({ + ...s, + accountsComments: { + ...s.accountsComments, + [accountId]: [ + ...(s.accountsComments?.[accountId] || []), + { + timestamp: Math.floor(Date.now() / 1000), + communityAddress: "test.eth", + content: "still retrying", + index: failedIndex, + publishingState: "failed", + state: "publishing", + }, + ], + }, + })); + const renderedPending = renderHook(() => useAccountComment({ commentIndex: failedIndex })); + const waitForPending = testUtils.createWaitFor(renderedPending); + await waitForPending(() => renderedPending.result.current.state === "pending"); + expect(renderedPending.result.current.state).toBe("pending"); + }); + + test("useAccountComment shows failed when publishingState is failed and publication state is stopped", async () => { + const state = accountsStore.getState() as any; + const accountId = state.activeAccountId || Object.keys(state.accounts)[0]; + const existing = state.accountsComments?.[accountId] || []; + const failedIndex = existing.length; + accountsStore.setState((s: any) => ({ + ...s, + accountsComments: { + ...s.accountsComments, + [accountId]: [ + ...(s.accountsComments?.[accountId] || []), + { + timestamp: Math.floor(Date.now() / 1000), + communityAddress: "test.eth", + content: "failed", + index: failedIndex, + publishingState: "failed", + state: "stopped", + }, + ], + }, + })); + const renderedFailed = renderHook(() => useAccountComment({ commentIndex: failedIndex })); + const waitForFailed = testUtils.createWaitFor(renderedFailed); + await waitForFailed(() => renderedFailed.result.current.state === "failed"); + expect(renderedFailed.result.current.state).toBe("failed"); }); test("useAccountVote with no commentCid returns initializing", async () => { diff --git a/src/hooks/accounts/accounts.ts b/src/hooks/accounts/accounts.ts index f242ccbb..9bb8b44e 100644 --- a/src/hooks/accounts/accounts.ts +++ b/src/hooks/accounts/accounts.ts @@ -312,7 +312,9 @@ export function useNotifications(options?: UseNotificationsOptions): UseNotifica } const getAccountCommentsStates = (accountComments: AccountComment[]) => { - // no longer consider an pending ater an expiry time of 20 minutes, consider failed + // Without a cid, the account comment is still a local pending publish. plebbit-js marks + // terminal publish failures when `publishingState === "failed"` and publication `state` + // is `"stopped"`, so we derive failed from that terminal pair or recorded publish errors. const now = Math.round(Date.now() / 1000); const expiryTime = now - 60 * 20; @@ -320,7 +322,20 @@ const getAccountCommentsStates = (accountComments: AccountComment[]) => { for (const accountComment of accountComments) { let state = "succeeded"; if (!accountComment.cid) { - if (accountComment.timestamp > expiryTime) { + const ac = accountComment as AccountComment & { + error?: Error; + errors?: Error[]; + publishingState?: string; + state?: string; + }; + const resolvedPublishFailed = + (ac.publishingState === "failed" && ac.state === "stopped") || + ac.error != null || + (Array.isArray(ac.errors) && ac.errors.length > 0); + + if (resolvedPublishFailed) { + state = "failed"; + } else if (accountComment.timestamp > expiryTime) { state = "pending"; } else { state = "failed"; @@ -447,7 +462,7 @@ export function useAccountComments(options?: UseAccountCommentsOptions): UseAcco parentCid, ]); - // recheck the states for changes every 1 minute because succeeded / failed / pending aren't events, they are time elapsed + // Recheck periodically so the 20-minute “stale pending → failed” transition updates without other store events const delay = 60_000; const immediate = false; useInterval( diff --git a/src/hooks/actions/actions.test.ts b/src/hooks/actions/actions.test.ts index b214cb1d..1e24bb7d 100644 --- a/src/hooks/actions/actions.test.ts +++ b/src/hooks/actions/actions.test.ts @@ -1007,15 +1007,14 @@ describe("actions", () => { }); // wait for error - await waitFor(() => rendered.result.current.errors.length === 2); - expect(rendered.result.current.errors.length).toBe(2); - expect(rendered.result.current.error.message).toBe("publish error"); + await waitFor(() => rendered.result.current.errors.length === 1); + expect(rendered.result.current.errors.length).toBe(1); + expect(rendered.result.current.error.message).toBe("emit error"); expect(rendered.result.current.errors[0].message).toBe("emit error"); - expect(rendered.result.current.errors[1].message).toBe("publish error"); // check callbacks + expect(onError).toHaveBeenCalledTimes(1); expect(onError.mock.calls[0][0].message).toBe("emit error"); - expect(onError.mock.calls[1][0].message).toBe("publish error"); // restore mock Comment.prototype.publish = commentPublish; diff --git a/src/stores/accounts/accounts-actions.test.ts b/src/stores/accounts/accounts-actions.test.ts index 65ad83cd..bcb1184e 100644 --- a/src/stores/accounts/accounts-actions.test.ts +++ b/src/stores/accounts/accounts-actions.test.ts @@ -1002,6 +1002,49 @@ describe("accounts-actions", () => { expect(comments.some((c: any) => c.cid)).toBe(true); }); + test("publishComment ignores stale errors from a replaced retry comment", async () => { + const account = Object.values(accountsStore.getState().accounts)[0]; + const createdComments: any[] = []; + const origCreateComment = account.plebbit.createComment.bind(account.plebbit); + vi.spyOn(account.plebbit, "createComment").mockImplementation(async (opts: any) => { + const comment = await origCreateComment(opts); + createdComments.push(comment); + return comment; + }); + + const onError = vi.fn(); + await act(async () => { + await accountsActions.publishComment({ + communityAddress: "sub.eth", + content: "retry stale error", + onChallenge: (ch: any, c: any) => c.publishChallengeAnswers(["4"]), + onChallengeVerification: () => {}, + onError, + }); + }); + + const start = Date.now(); + while (createdComments.length < 2 && Date.now() - start < 2000) { + await new Promise((resolve) => setTimeout(resolve, 25)); + } + + expect(createdComments).toHaveLength(2); + createdComments[0]?.listeners("error")?.[0]?.(new Error("stale retry error")); + await new Promise((resolve) => setTimeout(resolve, 50)); + + expect(onError).not.toHaveBeenCalled(); + const successStart = Date.now(); + while (Date.now() - successStart < 2000) { + const currentComments = accountsStore.getState().accountsComments[account.id] || []; + if (currentComments.some((comment: any) => comment.cid === "retry stale error cid")) { + break; + } + await new Promise((resolve) => setTimeout(resolve, 25)); + } + const comments = accountsStore.getState().accountsComments[account.id] || []; + expect(comments.some((comment: any) => comment.cid === "retry stale error cid")).toBe(true); + }); + test("publishVote retries on challenge failure", async () => { const opts = { communityAddress: "sub.eth", @@ -2146,7 +2189,7 @@ describe("accounts-actions", () => { expect(comments.find((c: any) => c.content === "ipfs")).toBeDefined(); }); - test("publishComment publish throws: onError called", async () => { + test("publishComment publish throws: stores error on the pending comment and calls onError", async () => { const account = Object.values(accountsStore.getState().accounts)[0]; const origCreate = account.plebbit.createComment.bind(account.plebbit); vi.spyOn(account.plebbit, "createComment").mockImplementation(async (opts: any) => { @@ -2168,6 +2211,51 @@ describe("accounts-actions", () => { await new Promise((r) => setTimeout(r, 100)); expect(onError).toHaveBeenCalled(); + const comments = accountsStore.getState().accountsComments[account.id] || []; + expect(comments[0]?.error?.message).toBe("publish failed"); + expect(comments[0]?.errors?.map((error: Error) => error.message)).toEqual(["publish failed"]); + }); + + test("publishComment stores terminal publication state and ignores later errors", async () => { + const account = Object.values(accountsStore.getState().accounts)[0]; + const origCreate = account.plebbit.createComment.bind(account.plebbit); + let commentRef: any; + let resolveCommentCreated!: () => void; + const commentCreated = new Promise((resolve) => { + resolveCommentCreated = resolve; + }); + vi.spyOn(account.plebbit, "createComment").mockImplementation(async (opts: any) => { + const c = await origCreate(opts); + commentRef = c; + resolveCommentCreated(); + vi.spyOn(c, "publish").mockResolvedValueOnce(undefined); + return c; + }); + + const onError = vi.fn(); + await act(async () => { + await accountsActions.publishComment({ + communityAddress: "sub.eth", + content: "terminal-state", + onChallenge: () => {}, + onChallengeVerification: () => {}, + onError, + }); + }); + + await commentCreated; + await act(async () => { + commentRef.emit("statechange", "stopped"); + commentRef.emit("publishingstatechange", "failed"); + }); + await Promise.resolve(); + commentRef.emit("error", new Error("late terminal error")); + await new Promise((resolve) => setTimeout(resolve, 25)); + + const comments = accountsStore.getState().accountsComments[account.id] || []; + expect(comments[0]?.state).toBe("stopped"); + expect(comments[0]?.publishingState).toBe("failed"); + expect(onError).not.toHaveBeenCalled(); }); test("publishComment startUpdatingAccountCommentOnCommentUpdateEvents error: catch logs (line 760)", async () => { diff --git a/src/stores/accounts/accounts-actions.ts b/src/stores/accounts/accounts-actions.ts index f9bd8246..b3e8503c 100644 --- a/src/stores/accounts/accounts-actions.ts +++ b/src/stores/accounts/accounts-actions.ts @@ -1066,140 +1066,212 @@ export const publishComment = async ( })(); let lastChallenge: Challenge | undefined; + let lastReportedPublishError: Error | undefined; + const normalizePublishError = (error: unknown): Error => + error instanceof Error ? error : new Error(String(error)); + const getActiveSessionForComment = (activeComment: any) => { + const session = getPublishSession(publishSessionId); + if ( + !session || + isPublishSessionAbandoned(publishSessionId) || + session.comment !== activeComment + ) { + return undefined; + } + return session; + }; + const queueCleanupFailedPublishSession = (activeComment: any) => { + if (!getActiveSessionForComment(activeComment)) return; + queueMicrotask(() => { + if (getActiveSessionForComment(activeComment)) { + cleanupPublishSessionOnTerminal(publishSessionId); + } + }); + }; + const recordPublishCommentError = (rawError: unknown, activeComment: any) => { + const error = normalizePublishError(rawError); + if (lastReportedPublishError === error) { + return error; + } + lastReportedPublishError = error; + + const session = getActiveSessionForComment(activeComment); + if (!session) return error; + const currentIndex = session.currentIndex; + accountsStore.setState(({ accountsComments }) => + maybeUpdateAccountComment(accountsComments, account.id, currentIndex, (ac, acc) => { + const previousErrors = Array.isArray(acc.errors) ? acc.errors : []; + const errors = + previousErrors[previousErrors.length - 1] === error + ? previousErrors + : [...previousErrors, error]; + ac[currentIndex] = { ...acc, errors, error }; + }), + ); + return error; + }; + const reportActivePublishCommentError = (rawError: unknown, activeComment: any) => { + if (!getActiveSessionForComment(activeComment)) return; + const error = recordPublishCommentError(rawError, activeComment); + queueCleanupFailedPublishSession(activeComment); + publishCommentOptions.onError?.(error, activeComment); + }; async function publishAndRetryFailedChallengeVerification() { if (isPublishSessionAbandoned(publishSessionId)) { return; } - updatePublishSessionComment(publishSessionId, comment); - comment.once("challenge", async (challenge: Challenge) => { + const activeComment = comment; + updatePublishSessionComment(publishSessionId, activeComment); + activeComment.once("challenge", async (challenge: Challenge) => { lastChallenge = challenge; - publishCommentOptions.onChallenge(challenge, comment); + publishCommentOptions.onChallenge(challenge, activeComment); }); - comment.once("challengeverification", async (challengeVerification: ChallengeVerification) => { - publishCommentOptions.onChallengeVerification(challengeVerification, comment); - if (!challengeVerification.challengeSuccess && lastChallenge) { - // publish again automatically on fail - const timestamp = Math.floor(Date.now() / 1000); - createCommentOptions = { ...createCommentOptions, timestamp }; - createdAccountComment = { ...createdAccountComment, timestamp }; - await saveCreatedAccountComment(createdAccountComment); - if (isPublishSessionAbandoned(publishSessionId)) { - return; - } - comment = backfillPublicationCommunityAddress( - await account.plebbit.createComment(createCommentOptions), - createCommentOptions, - ); - syncCommentClientsSnapshot(publishSessionId, account.id, comment); - lastChallenge = undefined; - publishAndRetryFailedChallengeVerification(); - } else { - // the challengeverification message of a comment publication should in theory send back the CID - // of the published comment which is needed to resolve it for replies, upvotes, etc - const session = getPublishSession(publishSessionId); - const currentIndex = session?.currentIndex ?? accountCommentIndex; - if (!session || isPublishSessionAbandoned(publishSessionId)) return; - queueMicrotask(() => cleanupPublishSessionOnTerminal(publishSessionId)); - if (challengeVerification?.commentUpdate?.cid) { - const persistedCommentWithCid = addShortAddressesToAccountComment( - sanitizeStoredAccountComment(normalizePublicationOptionsForStore(comment as any)), - ); - const liveCommentWithCid = addShortAddressesToAccountComment( - sanitizeAccountCommentForState(normalizePublicationOptionsForStore(comment as any)), - ); - delete (persistedCommentWithCid as any).clients; - delete (persistedCommentWithCid as any).publishingState; - delete (persistedCommentWithCid as any).error; - delete (persistedCommentWithCid as any).errors; - delete (liveCommentWithCid as any).clients; - delete (liveCommentWithCid as any).publishingState; - delete (liveCommentWithCid as any).error; - delete (liveCommentWithCid as any).errors; - await accountsDatabase.addAccountComment( - account.id, - persistedCommentWithCid, - currentIndex, + activeComment.once( + "challengeverification", + async (challengeVerification: ChallengeVerification) => { + publishCommentOptions.onChallengeVerification(challengeVerification, activeComment); + if (!challengeVerification.challengeSuccess && lastChallenge) { + // publish again automatically on fail + const timestamp = Math.floor(Date.now() / 1000); + createCommentOptions = { ...createCommentOptions, timestamp }; + createdAccountComment = { ...createdAccountComment, timestamp }; + updatePublishSessionComment(publishSessionId, undefined); + await saveCreatedAccountComment(createdAccountComment); + if (isPublishSessionAbandoned(publishSessionId)) { + return; + } + comment = backfillPublicationCommunityAddress( + await account.plebbit.createComment(createCommentOptions), + createCommentOptions, ); - accountsStore.setState( - ({ accountsComments, accountsCommentsIndexes, commentCidsToAccountsComments }) => { - const updatedAccountComments = [...accountsComments[account.id]]; - const updatedAccountComment = { - ...liveCommentWithCid, - index: currentIndex, - accountId: account.id, - }; - updatedAccountComments[currentIndex] = updatedAccountComment; - return { - accountsComments: { ...accountsComments, [account.id]: updatedAccountComments }, - accountsCommentsIndexes: { - ...accountsCommentsIndexes, - [account.id]: getAccountCommentsIndex(updatedAccountComments), - }, - commentCidsToAccountsComments: { - ...commentCidsToAccountsComments, - [challengeVerification?.commentUpdate?.cid]: { - accountId: account.id, - accountCommentIndex: currentIndex, + syncCommentClientsSnapshot(publishSessionId, account.id, comment); + lastChallenge = undefined; + publishAndRetryFailedChallengeVerification(); + } else { + // the challengeverification message of a comment publication should in theory send back the CID + // of the published comment which is needed to resolve it for replies, upvotes, etc + const session = getPublishSession(publishSessionId); + const currentIndex = session?.currentIndex ?? accountCommentIndex; + if (!session || isPublishSessionAbandoned(publishSessionId)) return; + queueMicrotask(() => cleanupPublishSessionOnTerminal(publishSessionId)); + if (challengeVerification?.commentUpdate?.cid) { + const persistedCommentWithCid = addShortAddressesToAccountComment( + sanitizeStoredAccountComment(normalizePublicationOptionsForStore(comment as any)), + ); + const liveCommentWithCid = addShortAddressesToAccountComment( + sanitizeAccountCommentForState(normalizePublicationOptionsForStore(comment as any)), + ); + delete (persistedCommentWithCid as any).clients; + delete (persistedCommentWithCid as any).publishingState; + delete (persistedCommentWithCid as any).error; + delete (persistedCommentWithCid as any).errors; + delete (liveCommentWithCid as any).clients; + delete (liveCommentWithCid as any).publishingState; + delete (liveCommentWithCid as any).error; + delete (liveCommentWithCid as any).errors; + await accountsDatabase.addAccountComment( + account.id, + persistedCommentWithCid, + currentIndex, + ); + accountsStore.setState( + ({ accountsComments, accountsCommentsIndexes, commentCidsToAccountsComments }) => { + const updatedAccountComments = [...accountsComments[account.id]]; + const updatedAccountComment = { + ...liveCommentWithCid, + index: currentIndex, + accountId: account.id, + }; + updatedAccountComments[currentIndex] = updatedAccountComment; + return { + accountsComments: { ...accountsComments, [account.id]: updatedAccountComments }, + accountsCommentsIndexes: { + ...accountsCommentsIndexes, + [account.id]: getAccountCommentsIndex(updatedAccountComments), }, - }, - }; - }, - ); + commentCidsToAccountsComments: { + ...commentCidsToAccountsComments, + [challengeVerification?.commentUpdate?.cid]: { + accountId: account.id, + accountCommentIndex: currentIndex, + }, + }, + }; + }, + ); - // clone the comment or it bugs publishing callbacks - const updatingComment = await account.plebbit.createComment( - normalizePublicationOptionsForPlebbit(account.plebbit, { ...comment }), - ); - accountsActionsInternal - .startUpdatingAccountCommentOnCommentUpdateEvents( - updatingComment, - account, - currentIndex, - ) - .catch((error: unknown) => - log.error( - "accountsActions.publishComment startUpdatingAccountCommentOnCommentUpdateEvents error", - { comment, account, accountCommentIndex, error }, - ), + // clone the comment or it bugs publishing callbacks + const updatingComment = await account.plebbit.createComment( + normalizePublicationOptionsForPlebbit(account.plebbit, { ...comment }), ); + accountsActionsInternal + .startUpdatingAccountCommentOnCommentUpdateEvents( + updatingComment, + account, + currentIndex, + ) + .catch((error: unknown) => + log.error( + "accountsActions.publishComment startUpdatingAccountCommentOnCommentUpdateEvents error", + { comment, account, accountCommentIndex, error }, + ), + ); + } } - } - }); + }, + ); - comment.on("error", (error: Error) => { - const session = getPublishSession(publishSessionId); - if (!session || isPublishSessionAbandoned(publishSessionId)) return; + activeComment.on("error", (error: Error) => { + reportActivePublishCommentError(error, activeComment); + }); + activeComment.on("statechange", (state: string) => { + const session = getActiveSessionForComment(activeComment); + if (!session) return; const currentIndex = session.currentIndex; + let hasTerminalFailedState = false; accountsStore.setState(({ accountsComments }) => maybeUpdateAccountComment(accountsComments, account.id, currentIndex, (ac, acc) => { - const errors = [...(acc.errors || []), error]; - ac[currentIndex] = { ...acc, errors, error }; + const nextAccountComment = { ...acc, state }; + ac[currentIndex] = nextAccountComment; + hasTerminalFailedState = + nextAccountComment.state === "stopped" && + nextAccountComment.publishingState === "failed"; }), ); - publishCommentOptions.onError?.(error, comment); + if (hasTerminalFailedState) { + queueCleanupFailedPublishSession(activeComment); + } }); - comment.on("publishingstatechange", async (publishingState: string) => { - const session = getPublishSession(publishSessionId); - if (!session || isPublishSessionAbandoned(publishSessionId)) return; + activeComment.on("publishingstatechange", async (publishingState: string) => { + const session = getActiveSessionForComment(activeComment); + if (!session) return; const currentIndex = session.currentIndex; + let hasTerminalFailedState = false; accountsStore.setState(({ accountsComments }) => maybeUpdateAccountComment(accountsComments, account.id, currentIndex, (ac, acc) => { - ac[currentIndex] = { ...acc, publishingState }; + const nextAccountComment = { ...acc, publishingState }; + ac[currentIndex] = nextAccountComment; + hasTerminalFailedState = + nextAccountComment.state === "stopped" && + nextAccountComment.publishingState === "failed"; }), ); + if (hasTerminalFailedState) { + queueCleanupFailedPublishSession(activeComment); + } publishCommentOptions.onPublishingStateChange?.(publishingState); }); // set clients on account comment so the frontend can display it, dont persist in db because a reload cancels publishing utils.clientsOnStateChange( - comment.clients, + activeComment.clients, (clientState: string, clientType: string, clientUrl: string, chainTicker?: string) => { - const session = getPublishSession(publishSessionId); - if (!session || isPublishSessionAbandoned(publishSessionId)) return; + const session = getActiveSessionForComment(activeComment); + if (!session) return; const currentIndex = session.currentIndex; accountsStore.setState(({ accountsComments }) => maybeUpdateAccountComment(accountsComments, account.id, currentIndex, (ac, acc) => { - const clients = getClientsSnapshotForState(comment.clients) || {}; + const clients = getClientsSnapshotForState(activeComment.clients) || {}; const client = { state: clientState }; if (chainTicker) { const chainProviders = { ...clients[clientType][chainTicker], [clientUrl]: client }; @@ -1213,13 +1285,13 @@ export const publishComment = async ( }, ); - listeners.push(comment); + listeners.push(activeComment); try { // publish will resolve after the challenge request // if it fails before, like failing to resolve ENS, we can emit the error - await comment.publish(); + await activeComment.publish(); } catch (error) { - publishCommentOptions.onError?.(error, comment); + reportActivePublishCommentError(error, activeComment); } }