fix(accounts): preserve challenge comment updates#60
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a sanitizer-aware helper to apply ChangesChallenge verification comment updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/stores/accounts/accounts-actions.test.ts (1)
520-528: 💤 Low valueConsider using a more robust wait utility instead of manual polling.
The manual polling loop works but could be replaced with a test utility like
waitForfrom@testing-library/reactor the existingtestUtils.createWaitFor. This would make the test more maintainable and reduce timeout/flakiness risks.♻️ Example using existing test utility
- 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 rendered = renderHook(() => accountsStore.getState().accountsComments[account.id]?.[0]); + const waitFor = testUtils.createWaitFor(rendered); + await waitFor(() => rendered.result.current?.reason === "AI moderation reason");🤖 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 520 - 528, Replace the manual polling loop that checks accountsStore.getState().accountsComments[account.id]?.[0] with a robust waiting utility (e.g., testing-library's waitFor or the repository's testUtils.createWaitFor). Specifically, remove the while loop and use waitFor to repeatedly assert that accountsStore.getState().accountsComments[account.id]?.[0]?.reason === "AI moderation reason" (or use createWaitFor to await that condition), ensuring you still wrap any state-updating calls in act if needed and set an appropriate timeout to avoid flakiness.src/stores/accounts/accounts-actions.ts (1)
80-96: ⚡ Quick winConsider stronger typing for the
publicationparameter.The
publication: anyparameter reduces type safety. Consider using a more specific type or a generic constraint to ensure compile-time validation.♻️ Suggested type improvement
-const applyChallengeVerificationCommentUpdateToPublication = ( - challengeVerification: ChallengeVerification | undefined, - publication: any, -) => { +const applyChallengeVerificationCommentUpdateToPublication = <T extends Record<string, unknown>>( + challengeVerification: ChallengeVerification | undefined, + publication: T, +): void => {Or define an explicit publication interface if the shape is known.
🤖 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.ts` around lines 80 - 96, The function applyChallengeVerificationCommentUpdateToPublication currently accepts publication: any which loses type safety; change its signature to use a specific Publication type or a generic like <T extends Record<string, unknown>> (or Publication | null) so the compiler enforces the expected shape, update the parameter in applyChallengeVerificationCommentUpdateToPublication to use that type/generic, and adjust any callers to pass the correctly typed object; ensure commentUpdate is typed (e.g., Partial<Publication>) so Object.assign(publication, commentUpdate) is type-safe.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/stores/accounts/accounts-actions.test.ts`:
- Around line 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.
In `@src/stores/accounts/accounts-actions.ts`:
- Around line 80-96: The function
applyChallengeVerificationCommentUpdateToPublication uses
Object.assign(publication, commentUpdate) which can cause prototype pollution if
commentUpdate is untrusted; instead, validate and copy only allowed own
properties from commentUpdate into publication (e.g., iterate
Object.keys(commentUpdate) and only set keys that are in an explicit allow-list
or that are not dangerous like "__proto__", "constructor", "prototype"), or
create a new object with a null prototype and shallow-merge only sanitized
fields; update applyChallengeVerificationCommentUpdateToPublication to perform
that safe copy and document the trust assumption if you rely on prior
sanitization.
---
Nitpick comments:
In `@src/stores/accounts/accounts-actions.test.ts`:
- Around line 520-528: Replace the manual polling loop that checks
accountsStore.getState().accountsComments[account.id]?.[0] with a robust waiting
utility (e.g., testing-library's waitFor or the repository's
testUtils.createWaitFor). Specifically, remove the while loop and use waitFor to
repeatedly assert that
accountsStore.getState().accountsComments[account.id]?.[0]?.reason === "AI
moderation reason" (or use createWaitFor to await that condition), ensuring you
still wrap any state-updating calls in act if needed and set an appropriate
timeout to avoid flakiness.
In `@src/stores/accounts/accounts-actions.ts`:
- Around line 80-96: The function
applyChallengeVerificationCommentUpdateToPublication currently accepts
publication: any which loses type safety; change its signature to use a specific
Publication type or a generic like <T extends Record<string, unknown>> (or
Publication | null) so the compiler enforces the expected shape, update the
parameter in applyChallengeVerificationCommentUpdateToPublication to use that
type/generic, and adjust any callers to pass the correctly typed object; ensure
commentUpdate is typed (e.g., Partial<Publication>) so
Object.assign(publication, commentUpdate) is type-safe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c311f87-421b-4cdf-8bb6-780cea83fc09
📒 Files selected for processing (2)
src/stores/accounts/accounts-actions.test.tssrc/stores/accounts/accounts-actions.ts
Comment on lines
+481
to
+556
| 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"; | ||
| publication.cid = cid; | ||
| publication.emit("challengeverification", { | ||
| type: "CHALLENGEVERIFICATION", | ||
| challengeRequestId: publication.challengeRequestId, | ||
| challengeAnswerId: publication.challengeAnswerId, | ||
| challengeSuccess: true, | ||
| commentUpdate: { | ||
| cid, | ||
| pendingApproval: true, | ||
| reason: "AI moderation reason", | ||
| removed: false, | ||
| }, | ||
| }); | ||
| 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, | ||
| }); | ||
|
|
||
| const persistedComments = await accountsDatabase.getAccountComments(account.id); | ||
| expect(persistedComments[0]).toMatchObject({ | ||
| cid: "moderated comment cid", | ||
| pendingApproval: true, | ||
| reason: "AI moderation reason", | ||
| removed: false, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ 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:
commentUpdateisnull,undefined, or an arraycommentUpdateis not an object (string, number, etc.)publicationisnull,undefined, or not an object- Valid
commentUpdatewith 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.
Member
Author
|
Addressed the valid security review finding in Triage notes: I did not export the private helper just to unit-test invalid helper inputs; the integration path is the behavior this package exposes, and the repo-wide hooks/stores coverage verifier is currently failing on pre-existing coverage gaps unrelated to this PR. The polling/type-safety comments were treated as non-blocking nits; the helper signature is now narrower than |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
challengeVerification.commentUpdatefields on published account commentsVerification
yarn prettieryarn buildyarn testNote: the repo-wide hooks/stores coverage verifier currently fails on pre-existing coverage gaps unrelated to this change.
Note
Medium Risk
Touches comment publish and persistence with security-sensitive filtering of
commentUpdate; scope is limited to accounts publish flow and covered by a new regression test.Overview
publishCommentnow mergeschallengeVerification.commentUpdateonto the in-flight publication and onto the cloned publication used forstartUpdatingAccountCommentOnCommentUpdateEvents, so moderation fields (e.g.pendingApproval,reason,cid) reach stored and persisted account comments instead of being dropped after challenge success.Merging uses a small helper that copies only own keys from
commentUpdateand skips__proto__,constructor, andprototypeto avoid prototype pollution from hostile update payloads.A regression test simulates a moderated challenge verification (including polluted enumerable keys) and asserts callbacks, in-memory store, and database all keep the expected metadata without unsafe properties.
Reviewed by Cursor Bugbot for commit d5eea80. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Tests
Bug Fixes