fix(communities): unwrap stats cid content#51
Conversation
📝 WalkthroughWalkthroughNormalize and safely parse various fetched CID shapes (raw JSON strings, wrapped Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
🧹 Nitpick comments (3)
src/hooks/communities.test.ts (2)
535-603: RestorefetchCidsafely even when the override call synchronously throws.In the three new tests the assignment
(PKC.prototype as any).fetchCid = () => …is performed before thetryblock, soorigFetchis captured but the restore infinallyonly runs for failures that occur afterrenderHookis invoked. This is consistent with the existing"fetchCid error logs"test, so it's not new debt — but since these stubs replace a prototype method used by every other test, consider moving the override insidetry(or usingvi.spyOn(PKC.prototype, "fetchCid").mockImplementation(...)withmockRestore()infinally) to make the cleanup bulletproof if the mock setup itself ever throws.♻️ Example using
vi.spyOn- const origFetch = PKC.prototype.fetchCid; - (PKC.prototype as any).fetchCid = () => - Promise.resolve({ - content: { hourActiveUserCount: 3, weekActiveUserCount: 33, allPostCount: 333 }, - }); - const rendered = renderHook<any, any>(() => - useCommunityStats({ community: { name: "wrapped stats" } }), - ); - const waitFor = testUtils.createWaitFor(rendered); - try { + const fetchSpy = vi.spyOn(PKC.prototype as any, "fetchCid").mockResolvedValue({ + content: { hourActiveUserCount: 3, weekActiveUserCount: 33, allPostCount: 333 }, + }); + try { + const rendered = renderHook<any, any>(() => + useCommunityStats({ community: { name: "wrapped stats" } }), + ); + const waitFor = testUtils.createWaitFor(rendered); await waitFor(() => rendered.result.current.state === "succeeded"); expect(rendered.result.current.hourActiveUserCount).toBe(3); expect(rendered.result.current.weekActiveUserCount).toBe(33); expect(rendered.result.current.allPostCount).toBe(333); } finally { - (PKC.prototype as any).fetchCid = origFetch; + fetchSpy.mockRestore(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/communities.test.ts` around lines 535 - 603, The tests override PKC.prototype.fetchCid before entering the try block which can leave the prototype patched if the setup throws; move the assignment of (PKC.prototype as any).fetchCid inside the try block (immediately before calling renderHook/useCommunityStats) so the finally block always restores origFetch, or replace the manual patch with vi.spyOn(PKC.prototype, "fetchCid").mockImplementation(...) and call .mockRestore() in the finally block to guarantee safe restoration even on synchronous failures.
583-603: Third test doesn't actually exercise a new code path vs. the existinguseCommunityStatstest.
test("useCommunityStats accepts object fetchCid responses", …)resolvesfetchCidwith a plain stats object. The existing default mock inpkc-js-mock-content.tsreturns a JSON-stringified stats object, and the priortest("useCommunityStats", …)already covers the "raw stringified JSON" path throughparseMaybeJson. The three added tests together still leave the string-at-top-level branch (i.e.fetchCidreturningJSON.stringify({...})directly) implicitly covered only by the default mock path. Worth adding an explicit test for that branch too so regressions inparseMaybeJsoncan't silently be masked by one of the wrapped-content tests, keeping the 100% hooks coverage requirement robust.As per coding guidelines: "Maintain mandatory 100% test coverage for hooks and stores in
src/hooks/andsrc/stores/; every feature or bug fix must include/adjust tests to keep coverage at 100%".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/communities.test.ts` around lines 583 - 603, Add a new test that explicitly exercises the "string-at-top-level" branch by stubbing PKC.prototype.fetchCid to return Promise.resolve(JSON.stringify({ hourActiveUserCount: 5, weekActiveUserCount: 55, allPostCount: 555 })), render the useCommunityStats hook (useCommunityStats({ community: { name: "string stats" } })), wait until state === "succeeded", assert the three count fields match 5/55/555, and restore the original PKC.prototype.fetchCid in a finally block so the prior tests and default mock remain unchanged; name the test to reflect the string-response case.src/hooks/communities.ts (1)
31-37: Consider guarding against non-objectcontentvalues.
parseFetchedCommunityStatsunwrapscontentwhenever the"content" in parsedCidcheck passes, even ifcontentisundefined,null, or a primitive. For a payload like{ content: undefined }the hook would writeundefinedinto the stats store and causeuseCommunityStatsto stay in"fetching-ipfs"indefinitely rather than falling through to the direct-object branch. Also note that ifparsedCid.contentis a string that isn't valid JSON,JSON.parsewill throw and bubble up as a fetch error — probably fine given the existingtry/catch, but worth being intentional.♻️ Suggested tightening
const parseFetchedCommunityStats = (fetchedCid: unknown): CommunityStats => { const parsedCid = parseMaybeJson(fetchedCid); - if (parsedCid && typeof parsedCid === "object" && "content" in parsedCid) { - return parseMaybeJson((parsedCid as { content: unknown }).content) as CommunityStats; + if ( + parsedCid && + typeof parsedCid === "object" && + "content" in parsedCid && + (parsedCid as { content: unknown }).content != null + ) { + return parseMaybeJson((parsedCid as { content: unknown }).content) as CommunityStats; } return parsedCid as CommunityStats; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/communities.ts` around lines 31 - 37, parseFetchedCommunityStats currently unwraps parsedCid.content whenever the "content" key exists, which can write undefined/null/primitives into the stats store; update the function (reference: parseFetchedCommunityStats, parsedCid, parsedCid.content) to only unwrap when parsedCid.content is non-null/undefined and is either an object or a JSON string that successfully parses to an object via parseMaybeJson; otherwise return parsedCid as the CommunityStats. Ensure you call parseMaybeJson on the content and verify the result is an object before casting/returning to avoid injecting invalid values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/hooks/communities.test.ts`:
- Around line 535-603: The tests override PKC.prototype.fetchCid before entering
the try block which can leave the prototype patched if the setup throws; move
the assignment of (PKC.prototype as any).fetchCid inside the try block
(immediately before calling renderHook/useCommunityStats) so the finally block
always restores origFetch, or replace the manual patch with
vi.spyOn(PKC.prototype, "fetchCid").mockImplementation(...) and call
.mockRestore() in the finally block to guarantee safe restoration even on
synchronous failures.
- Around line 583-603: Add a new test that explicitly exercises the
"string-at-top-level" branch by stubbing PKC.prototype.fetchCid to return
Promise.resolve(JSON.stringify({ hourActiveUserCount: 5, weekActiveUserCount:
55, allPostCount: 555 })), render the useCommunityStats hook
(useCommunityStats({ community: { name: "string stats" } })), wait until state
=== "succeeded", assert the three count fields match 5/55/555, and restore the
original PKC.prototype.fetchCid in a finally block so the prior tests and
default mock remain unchanged; name the test to reflect the string-response
case.
In `@src/hooks/communities.ts`:
- Around line 31-37: parseFetchedCommunityStats currently unwraps
parsedCid.content whenever the "content" key exists, which can write
undefined/null/primitives into the stats store; update the function (reference:
parseFetchedCommunityStats, parsedCid, parsedCid.content) to only unwrap when
parsedCid.content is non-null/undefined and is either an object or a JSON string
that successfully parses to an object via parseMaybeJson; otherwise return
parsedCid as the CommunityStats. Ensure you call parseMaybeJson on the content
and verify the result is an object before casting/returning to avoid injecting
invalid values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 218e9ee6-c281-45f0-a6d0-8a41f8228cfc
📒 Files selected for processing (2)
src/hooks/communities.test.tssrc/hooks/communities.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Addressed review feedback in the latest commit: tightened stats wrapper detection so direct stats payloads with a content field are preserved, switched the new tests to spy cleanup, and added explicit coverage for top-level JSON string stats responses. Local verification: prettier, focused communities test, build, lint, and type-check passed. Full test and hooks/stores coverage runs still hit the existing Vitest/Node heap OOM around hooks/accounts/accounts.test.ts on this machine. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 80ac8a3. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/hooks/communities.ts (1)
29-37: Top-levelparseMaybeJsonthrows on non-JSON strings — consider usingtryParseMaybeJson.At line 47,
parseMaybeJson(fetchedCid)will throwSyntaxErroriffetchedCidis a non-JSON string (e.g., a raw CID content that isn't JSON). That propagates out ofparseFetchedCommunityStatsinto the outertry/catchat line 215-222 and is reported as a fetch failure. Thecontent-field branch already usestryParseMaybeJsonfor resilience; applying the same here would let non-JSON top-level strings at least flow through to the fallback return without being treated as a hard failure.const parseFetchedCommunityStats = (fetchedCid: unknown): CommunityStats => { - const parsedCid = parseMaybeJson(fetchedCid); + const parsedCid = tryParseMaybeJson(fetchedCid);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/communities.ts` around lines 29 - 37, The top-level call to parseMaybeJson for fetchedCid in parseFetchedCommunityStats can throw on non-JSON strings; change that usage to tryParseMaybeJson(fetchedCid) so non-JSON CID strings are returned as-is rather than triggering a hard SyntaxError, mirroring the resilience already used for the content branch; update the reference in parseFetchedCommunityStats where fetchedCid is parsed to call tryParseMaybeJson and ensure all downstream code still treats the returned value the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/communities.ts`:
- Around line 46-58: The current parseFetchedCommunityStats function can return
a value that doesn't match CommunityStats, causing silent undefined fields;
update parseFetchedCommunityStats (and its uses of parseMaybeJson,
tryParseMaybeJson, isCommunityStatsPayload) to detect the unrecognized shape and
instead surface an error: throw a descriptive Error (including the raw parsed
payload/context) so the outer fetch catch sets state to "failed" and triggers
setFetchError; alternatively, if you prefer not to throw, emit a process/local
logger.warn with the payload and return a safe default, but the preferred fix is
to throw to avoid transitioning to "succeeded" with invalid data.
---
Nitpick comments:
In `@src/hooks/communities.ts`:
- Around line 29-37: The top-level call to parseMaybeJson for fetchedCid in
parseFetchedCommunityStats can throw on non-JSON strings; change that usage to
tryParseMaybeJson(fetchedCid) so non-JSON CID strings are returned as-is rather
than triggering a hard SyntaxError, mirroring the resilience already used for
the content branch; update the reference in parseFetchedCommunityStats where
fetchedCid is parsed to call tryParseMaybeJson and ensure all downstream code
still treats the returned value the same.
🪄 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: edec981c-10ba-46a0-8673-fd96884f23b0
📒 Files selected for processing (2)
src/hooks/communities.test.tssrc/hooks/communities.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/hooks/communities.test.ts
| const parseFetchedCommunityStats = (fetchedCid: unknown): CommunityStats => { | ||
| const parsedCid = parseMaybeJson(fetchedCid); | ||
| if (isCommunityStatsPayload(parsedCid)) { | ||
| return parsedCid; | ||
| } | ||
| if (isRecord(parsedCid) && "content" in parsedCid) { | ||
| const parsedContent = tryParseMaybeJson(parsedCid.content); | ||
| if (isCommunityStatsPayload(parsedContent)) { | ||
| return parsedContent; | ||
| } | ||
| } | ||
| return parsedCid as CommunityStats; | ||
| }; |
There was a problem hiding this comment.
Fallback silently stores non-stats payloads as CommunityStats.
When neither the top-level payload nor parsedCid.content matches isCommunityStatsPayload, line 57 returns parsedCid as CommunityStats and the hook transitions to state === "succeeded" with a value that doesn't actually have any of the expected stat fields. Consumers reading hourActiveUserCount/weekActiveUserCount/allPostCount will silently see undefined instead of surfacing a fetch error or staying in a fetching state.
Consider either throwing (so the outer catch routes to setFetchError / state: "failed") or at least logging a warning when the shape is unrecognized:
Proposed change
- return parsedCid as CommunityStats;
+ log.error?.("parseFetchedCommunityStats: unrecognized payload shape", { fetchedCid });
+ throw new Error("useCommunityStats received unrecognized stats payload shape");
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/communities.ts` around lines 46 - 58, The current
parseFetchedCommunityStats function can return a value that doesn't match
CommunityStats, causing silent undefined fields; update
parseFetchedCommunityStats (and its uses of parseMaybeJson, tryParseMaybeJson,
isCommunityStatsPayload) to detect the unrecognized shape and instead surface an
error: throw a descriptive Error (including the raw parsed payload/context) so
the outer fetch catch sets state to "failed" and triggers setFetchError;
alternatively, if you prefer not to throw, emit a process/local logger.warn with
the payload and return a safe default, but the preferred fix is to throw to
avoid transitioning to "succeeded" with invalid data.
1 participant
Summary
fetchCidstats responses that now return an object withcontentVerification
corepack yarn prettiercorepack yarn test src/hooks/communities.test.tscorepack yarn buildcorepack yarn lint(passes with existing warnings)corepack yarn type-checkcorepack yarn test:coverage:hooks-storesfailed twice from Node/Vitest worker heap exhaustion after running the suite; retry with a single forked worker still ended with OOM after 35/36 files and 1011/1047 tests passedNote
Medium Risk
Changes how
useCommunityStatsparsespkc.fetchCidresponses, which could surface new runtime parsing behavior (e.g., unexpected non-JSON strings) but is limited to stats fetching logic and is well-covered by new tests.Overview
useCommunityStatsnow normalizes community stats returned bypkc.fetchCid, handling both legacy raw JSON strings/direct objects and newer responses that wrap stats under acontentfield (includingcontentas a JSON string).Adds regression tests covering wrapped object stats, wrapped string stats, raw string responses, direct object payloads, and ensuring unknown/extra
contentis preserved.Reviewed by Cursor Bugbot for commit 0bf5cbb. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Tests
Bug Fixes