Skip to content

fix(accounts): preserve challenge comment updates #60

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tomcasaburi merged 2 commits into from
Jun 4, 2026
Merged

fix(accounts): preserve challenge comment updates #60

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3148cd3
fix(accounts): preserve challenge comment updates
tomcasaburi Jun 4, 2026
d5eea80
fix(accounts): harden challenge comment updates
tomcasaburi Jun 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 90 additions & 0 deletions src/stores/accounts/accounts-actions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,96 @@ describe("accounts-actions", () => {
expect(comments.some((c: any) => c.content === "from named account")).toBe(true);
});

test("publishComment preserves challenge commentUpdate fields on the account comment", async () => {
await act(async () => {
await accountsActions.createAccount();
});
const account = Object.values(accountsStore.getState().accounts)[0];
const createComment = account.pkc.createComment.bind(account.pkc);
vi.spyOn(account.pkc, "createComment").mockImplementation(async (opts: any) => {
const publication: any = await createComment(opts);
vi.spyOn(publication, "simulateChallengeVerificationEvent").mockImplementation(() => {
const cid = "moderated comment cid";
const commentUpdate: any = {
cid,
pendingApproval: true,
reason: "AI moderation reason",
removed: false,
};
Object.defineProperties(commentUpdate, {
__proto__: { value: { polluted: true }, enumerable: true },
constructor: { value: { polluted: true }, enumerable: true },
prototype: { value: { polluted: true }, enumerable: true },
});
publication.cid = cid;
publication.emit("challengeverification", {
type: "CHALLENGEVERIFICATION",
challengeRequestId: publication.challengeRequestId,
challengeAnswerId: publication.challengeAnswerId,
challengeSuccess: true,
commentUpdate,
});
publication.publishingState = "succeeded";
publication.emit("publishingstatechange", "succeeded");
});
return publication;
});
const onChallengeVerification = vi.fn();

await act(async () => {
await accountsActions.publishComment({
communityAddress: "sub.eth",
content: "moderated comment",
onChallenge: (challenge: any, comment: any) => comment.publishChallengeAnswers(["4"]),
onChallengeVerification,
});
});

const startedAt = Date.now();
while (Date.now() - startedAt < 2000) {
await act(async () => {});
const comment = accountsStore.getState().accountsComments[account.id]?.[0];
if (comment?.reason === "AI moderation reason") {
break;
}
await new Promise((resolve) => setTimeout(resolve, 25));
}

const storedComment = accountsStore.getState().accountsComments[account.id]?.[0];
expect(onChallengeVerification).toHaveBeenCalledWith(
expect.objectContaining({
commentUpdate: expect.objectContaining({ reason: "AI moderation reason" }),
}),
expect.objectContaining({
cid: "moderated comment cid",
pendingApproval: true,
reason: "AI moderation reason",
removed: false,
}),
);
expect(storedComment).toMatchObject({
cid: "moderated comment cid",
pendingApproval: true,
reason: "AI moderation reason",
removed: false,
});
expect(Object.prototype.hasOwnProperty.call(storedComment, "__proto__")).toBe(false);
expect(Object.prototype.hasOwnProperty.call(storedComment, "constructor")).toBe(false);
expect(Object.prototype.hasOwnProperty.call(storedComment, "prototype")).toBe(false);
expect(({} as any).polluted).toBeUndefined();

const persistedComments = await accountsDatabase.getAccountComments(account.id);
expect(persistedComments[0]).toMatchObject({
cid: "moderated comment cid",
pendingApproval: true,
reason: "AI moderation reason",
removed: false,
});
expect(Object.prototype.hasOwnProperty.call(persistedComments[0], "__proto__")).toBe(false);
expect(Object.prototype.hasOwnProperty.call(persistedComments[0], "constructor")).toBe(false);
expect(Object.prototype.hasOwnProperty.call(persistedComments[0], "prototype")).toBe(false);
});
Comment on lines +481 to +569

@coderabbitai coderabbitai Bot Jun 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add direct unit tests for the helper function's edge cases.

The test validates the integration path but doesn't directly test applyChallengeVerificationCommentUpdateToPublication's validation logic (lines 85-92 in accounts-actions.ts). Per coding guidelines, stores require mandatory 100% test coverage.

Add test cases covering:

  • commentUpdate is null, undefined, or an array
  • commentUpdate is not an object (string, number, etc.)
  • publication is null, undefined, or not an object
  • Valid commentUpdate with nested objects (to document shallow-merge behavior)

Based on learnings: "Maintain mandatory 100% test coverage for hooks and stores (src/hooks/, src/stores/); every feature or bug fix in these areas must include/adjust tests to keep coverage at 100%, verified via coverage run + node scripts/verify-hooks-stores-coverage.mjs"

📋 Example edge-case tests 
test("applyChallengeVerificationCommentUpdateToPublication handles invalid commentUpdate", () => {
  const publication = { content: "test" };
  
  // Should not throw or mutate when commentUpdate is invalid
  applyChallengeVerificationCommentUpdateToPublication(undefined, publication);
  applyChallengeVerificationCommentUpdateToPublication({ commentUpdate: null } as any, publication);
  applyChallengeVerificationCommentUpdateToPublication({ commentUpdate: [] } as any, publication);
  applyChallengeVerificationCommentUpdateToPublication({ commentUpdate: "invalid" } as any, publication);
  
  expect(publication).toEqual({ content: "test" });
});

test("applyChallengeVerificationCommentUpdateToPublication handles invalid publication", () => {
  const commentUpdate = { cid: "test", reason: "test" };
  
  // Should not throw when publication is invalid
  applyChallengeVerificationCommentUpdateToPublication({ commentUpdate } as any, null);
  applyChallengeVerificationCommentUpdateToPublication({ commentUpdate } as any, undefined);
  applyChallengeVerificationCommentUpdateToPublication({ commentUpdate } as any, "invalid" as any);
});

Note: You may need to export the helper function for testing, or add an integration test that exercises these branches via publishComment.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/stores/accounts/accounts-actions.test.ts` around lines 481 - 556, Add
direct unit tests that exercise
applyChallengeVerificationCommentUpdateToPublication's validation branches:
create tests calling applyChallengeVerificationCommentUpdateToPublication with
commentUpdate values null, undefined, [], non-object types (string/number), and
with publication values null, undefined, or non-object to assert no throw and no
unintended mutation; also add a test that passes a valid nested commentUpdate to
document the shallow-merge behavior. Export or import
applyChallengeVerificationCommentUpdateToPublication from accounts-actions.ts in
the test file and add assertions that the publication object remains unchanged
for invalid inputs and is shallow-merged for valid inputs to satisfy 100%
store/hooks coverage.


test("publishVote with accountName uses named account", async () => {
await act(async () => {
await accountsActions.createAccount();
Expand Down
29 changes: 29 additions & 0 deletions src/stores/accounts/accounts-actions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,30 @@ const getClientsSnapshotForState = (clients: any): any => {
return Object.keys(snapshot).length > 0 ? snapshot : undefined;
};

const unsafeCommentUpdatePropertyNames = new Set(["__proto__", "constructor", "prototype"]);

const applyChallengeVerificationCommentUpdateToPublication = (
challengeVerification: ChallengeVerification | undefined,
publication: Record<string, any> | undefined,
) => {
const commentUpdate = challengeVerification?.commentUpdate;
if (
!commentUpdate ||
typeof commentUpdate !== "object" ||
Array.isArray(commentUpdate) ||
!publication ||
typeof publication !== "object"
) {
return;
}

for (const key of Object.keys(commentUpdate)) {
if (!unsafeCommentUpdatePropertyNames.has(key)) {
publication[key] = commentUpdate[key];
}
}
};
Comment thread
coderabbitai[bot] marked this conversation as resolved.

const syncCommentClientsSnapshot = (
publishSessionId: string,
accountId: string,
Expand Down Expand Up @@ -1129,6 +1153,7 @@ export const publishComment = async (
activeComment.once(
"challengeverification",
async (challengeVerification: ChallengeVerification) => {
applyChallengeVerificationCommentUpdateToPublication(challengeVerification, activeComment);
publishCommentOptions.onChallengeVerification(challengeVerification, activeComment);
if (!challengeVerification.challengeSuccess && lastChallenge) {
// publish again automatically on fail
Expand Down Expand Up @@ -1204,6 +1229,10 @@ export const publishComment = async (
const updatingComment = await account.pkc.createComment(
normalizePublicationOptionsForPkc(account.pkc, { ...comment }),
);
applyChallengeVerificationCommentUpdateToPublication(
challengeVerification,
updatingComment,
);
accountsActionsInternal
.startUpdatingAccountCommentOnCommentUpdateEvents(
updatingComment,
Expand Down
Loading