Add block filtering to store pages - #184
Conversation
|
Warning Review limit reachedNext included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds store-owner block/unblock feature: new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Page as Item/Store/Cart Page
participant Hook as useBlock Hook
participant API as Backend API
participant UI as Page UI
User->>Page: Click "Unblock" button
Page->>Hook: toggleBlock()
Hook->>API: send block/unblock request
API-->>Hook: respond success/failure
Hook->>UI: update isLoading and isBlocked
UI-->>Page: re-render banners/buttons
Page-->>User: banner updates/disappears
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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: 1
🤖 Fix all issues with AI agents
In `@app/store/page.tsx`:
- Around line 82-96: The async checkBlockedOwners inside the useEffect can
complete after user or stores change and overwrite blockedOwners with stale
results; modify the effect to use a cancellation guard (e.g., a local `let
cancelled = false` or an AbortController/token) scoped inside the effect, pass
the token to the async call if supported (checkBlockedForAuthors) and before
calling setBlockedOwners verify the request is not cancelled (e.g., if
(!cancelled) setBlockedOwners(blocked)); also ensure you flip the cancellation
flag in the effect cleanup to prevent stale updates when user.identityId or
stores change or on logout.
Deploying yappr with
|
| Latest commit: |
292685f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2de224cd.yappr.pages.dev |
| Branch Preview URL: | https://claude-review-store-blocklis.yappr.pages.dev |
- Filter blocked users' stores from store browse page - Show blocked banner on store detail and item detail pages with unblock option - Add warning in cart for items from blocked store owners - Reuse existing checkBlockedForAuthors and useBlock hook patterns https://claude.ai/code/session_01Kry9GcgBhrpJuc8pR5CdcR
Add cancellation guard to prevent stale async results from overwriting blockedOwners state when user.identityId or stores change during fetch. This addresses a CodeRabbit review finding where checkBlockedForAuthors completing after a logout or store list change could set incorrect data. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
24a5480 to
605489f
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
⛔ Blockers found — Opus deferred (commit 292685f) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The PR adds store filtering and block-state warnings, but two core correctness gaps remain. Blocked stores can render while their status is unresolved, and inherited blocks are presented as directly unblockable even though the action cannot remove them.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `app/store/page.tsx`:
- [BLOCKING] app/store/page.tsx:111-114: Wait for block status before rendering stores
`blockedOwners` is initially empty, and the independent block-status effect does not participate in `isLoading`. Consequently, when ratings finish before the block query—or the authenticated identity changes after the list is visible—these lines treat unresolved status as if no owners were blocked and render every store as clickable. Keep the list loading or hidden until the block check for the current identity and store set completes.
In `app/store/view/page.tsx`:
- [BLOCKING] app/store/view/page.tsx:322-329: Do not treat inherited blocks as directly unblockable
`useBlock` derives its boolean from `blockService.isBlocked`, which includes blocks inherited from followed block lists. However, `toggleBlock` handles every true value with `unblockUser(viewerId, ownerId)`, which only deletes the viewer's own block and returns success when no such document exists. For an inherited block, the UI therefore shows a success toast and optimistically hides the banner even though the owner remains blocked and browse filtering will rediscover the block. The same control exists in `app/item/page.tsx`; expose block provenance and recompute the combined status after removing any direct block, while routing inherited-only cases to the relevant block-list settings.
| // Filter out stores owned by blocked users | ||
| if (blockedOwners.size > 0) { | ||
| filtered = filtered.filter(store => !blockedOwners.get(store.ownerId)) | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Wait for block status before rendering stores
blockedOwners is initially empty, and the independent block-status effect does not participate in isLoading. Consequently, when ratings finish before the block query—or the authenticated identity changes after the list is visible—these lines treat unresolved status as if no owners were blocked and render every store as clickable. Keep the list loading or hidden until the block check for the current identity and store set completes.
source: ['codex']
| <Button | ||
| variant="outline" | ||
| size="sm" | ||
| onClick={() => toggleBlock()} | ||
| disabled={isBlockLoading} | ||
| className="border-red-300 dark:border-red-700 text-red-700 dark:text-red-400 hover:bg-red-100 dark:hover:bg-red-900/40" | ||
| > | ||
| Unblock |
There was a problem hiding this comment.
🔴 Blocking: Do not treat inherited blocks as directly unblockable
useBlock derives its boolean from blockService.isBlocked, which includes blocks inherited from followed block lists. However, toggleBlock handles every true value with unblockUser(viewerId, ownerId), which only deletes the viewer's own block and returns success when no such document exists. For an inherited block, the UI therefore shows a success toast and optimistically hides the banner even though the owner remains blocked and browse filtering will rediscover the block. The same control exists in app/item/page.tsx; expose block provenance and recompute the combined status after removing any direct block, while routing inherited-only cases to the relevant block-list settings.
source: ['codex']
# Conflicts: # app/store/page.tsx # app/store/view/page.tsx
…ted blocks useBlock's isBlocked included blocks inherited from followed block lists, but toggleBlock only deleted the viewer's own block document, so unblocking an inherited block showed a success toast while the owner stayed blocked. blockService.getBlockProvenance now resolves own and inherited block sources, useBlock exposes isOwnBlock/inheritedFrom, and the store view, item, and cart banners route inherited-only blocks to block list settings instead of offering a no-op Unblock. Removing an own block that is also list-inherited keeps the banner in the still-blocked state. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
blockedOwners started empty and was not part of isLoading, so blocked stores flashed as clickable before the block check finished. The browse page now tracks which (identity, store set) the block map was resolved for and keeps the loading state until it matches, so blocked stores never render prematurely. Guests skip the check entirely and see no added latency. Errors fail open and use the shared logger. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…al surfaces Resolving provenance with a live query on every mount regressed the batch-prefetch guarantee post-card relies on to avoid N+1 DAPI queries. useBlock now derives provenance from the sessionStorage confirmed-block cache (which records who blocked the target) when available, and only the store view and item pages opt into an exact platform lookup via the new resolveProvenance option, since their banners must distinguish an own block from one also inherited via a followed list. The inherited-only unblock refusal now requires known provenance, so a click during the brief unresolved window falls back to attempting an own unblock instead of wrongly refusing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Without root: true, ESLint cascades past the repo when it is checked out inside a nested directory (e.g. a git worktree under another checkout) and conflicts with any parent .eslintrc.json. Marking the config as root makes linting behave identically regardless of where the repo is checked out. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Deploying yappr-v2 with
|
| Latest commit: |
292685f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://80ac6aa6.yappr-v2.pages.dev |
| Branch Preview URL: | https://claude-review-store-blocklis.yappr-v2.pages.dev |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The PR adds the intended store filtering and provenance-aware warnings, but two cache-invalidation gaps can still expose blocked stores or suppress newly confirmed block warnings. The cart implementation also scales Platform reads by both cart-store count and followed block-list count instead of batching owners. Source: reviewer backend gpt-5.6-sol; final verifier backend claude-opus-4-6; orchestration-only (not reviewer evidence): openclaw-agent/cliproxy/gpt-5.6-sol.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `app/store/page.tsx`:
- [BLOCKING] app/store/page.tsx:91-99: Invalidate a previously resolved key before rechecking it
`blockedResolvedKey` survives both the unauthenticated branch and the start of a new request. After identity A resolves key K, an A → guest → A transition with the same stores clears `blockedOwners` but leaves K marked as resolved. On A's return, `isBlockCheckPending` is therefore false throughout the replacement query, so every store—including A's blocked stores—renders as clickable. Clear the resolved key whenever this effect invalidates the map or begins a check, ensuring an old result cannot certify a later request with the same key.
In `lib/services/block-service.ts`:
- [BLOCKING] lib/services/block-service.ts:651-673: Preserve confirmed own blocks while resolving provenance
`blockUser` records a successful block in the own-block cache, and `isBlocked` treats that cache as authoritative, but `getBlockProvenance` ignores it. Immediately after a successful broadcast, the new document may not yet be queryable—or `getBlock` may convert a transient read failure into `null`. This method then overwrites the confirmed positive entry with `false`, causing the newly added store, item, and cart warnings to disappear even though the direct block is known to have succeeded. Preserve the cached direct block while still querying followed lists for inherited provenance.
In `components/store/cart-store-section.tsx`:
- [SUGGESTION] components/store/cart-store-section.tsx:25-26: Batch cart-owner block checks before rendering sections
Each cart store independently mounts `useBlock`. Without a confirmed result, the hook calls `getBlockProvenance`, which performs a direct-block query and then one query for every followed block list. A cart containing S stores while the viewer follows F block lists can therefore issue approximately S × (F + 1) Platform reads, even when the merged bloom filter proves that most owners are not blocked. Batch all cart owner IDs once with `checkBlockedForAuthors` in the cart page and pass the prefetched status into each section; `checkBlockedBatch` will seed the confirmed provenance cache consumed by these hooks.
| useEffect(() => { | ||
| if (!user?.identityId || stores.length === 0) { | ||
| setBlockedOwners(new Map()) | ||
| return | ||
| } | ||
|
|
||
| let cancelled = false | ||
| const identityId = user.identityId | ||
| const key = blockCheckKey |
There was a problem hiding this comment.
🔴 Blocking: Invalidate a previously resolved key before rechecking it
blockedResolvedKey survives both the unauthenticated branch and the start of a new request. After identity A resolves key K, an A → guest → A transition with the same stores clears blockedOwners but leaves K marked as resolved. On A's return, isBlockCheckPending is therefore false throughout the replacement query, so every store—including A's blocked stores—renders as clickable. Clear the resolved key whenever this effect invalidates the map or begins a check, ensuring an old result cannot certify a later request with the same key.
| useEffect(() => { | |
| if (!user?.identityId || stores.length === 0) { | |
| setBlockedOwners(new Map()) | |
| return | |
| } | |
| let cancelled = false | |
| const identityId = user.identityId | |
| const key = blockCheckKey | |
| useEffect(() => { | |
| setBlockedResolvedKey('') | |
| if (!user?.identityId || stores.length === 0) { | |
| setBlockedOwners(new Map()) | |
| return | |
| } | |
| let cancelled = false | |
| const identityId = user.identityId | |
| const key = blockCheckKey |
source: ['codex']
| const [ownBlock, followedBlockers] = await Promise.all([ | ||
| this.getBlock(targetUserId, viewerId), | ||
| this.getBlockFollows(viewerId) | ||
| ]) | ||
|
|
||
| const inherited = followedBlockers.length > 0 | ||
| ? await this.checkInheritedBlocks(targetUserId, followedBlockers) | ||
| : null | ||
|
|
||
| // Keep the confirmed-block cache in sync (own block takes precedence, | ||
| // matching isBlocked() behavior) | ||
| if (ownBlock) { | ||
| addConfirmedBlock(viewerId, targetUserId, viewerId, true, ownBlock.message) | ||
| } else if (inherited) { | ||
| addConfirmedBlock(viewerId, targetUserId, inherited.blockedBy, true, inherited.message) | ||
| } else { | ||
| addConfirmedBlock(viewerId, targetUserId, '', false) | ||
| } | ||
|
|
||
| return { | ||
| isBlocked: Boolean(ownBlock) || inherited !== null, | ||
| isOwnBlock: Boolean(ownBlock), | ||
| inheritedFrom: inherited?.blockedBy ?? null |
There was a problem hiding this comment.
🔴 Blocking: Preserve confirmed own blocks while resolving provenance
blockUser records a successful block in the own-block cache, and isBlocked treats that cache as authoritative, but getBlockProvenance ignores it. Immediately after a successful broadcast, the new document may not yet be queryable—or getBlock may convert a transient read failure into null. This method then overwrites the confirmed positive entry with false, causing the newly added store, item, and cart warnings to disappear even though the direct block is known to have succeeded. Preserve the cached direct block while still querying followed lists for inherited provenance.
| const [ownBlock, followedBlockers] = await Promise.all([ | |
| this.getBlock(targetUserId, viewerId), | |
| this.getBlockFollows(viewerId) | |
| ]) | |
| const inherited = followedBlockers.length > 0 | |
| ? await this.checkInheritedBlocks(targetUserId, followedBlockers) | |
| : null | |
| // Keep the confirmed-block cache in sync (own block takes precedence, | |
| // matching isBlocked() behavior) | |
| if (ownBlock) { | |
| addConfirmedBlock(viewerId, targetUserId, viewerId, true, ownBlock.message) | |
| } else if (inherited) { | |
| addConfirmedBlock(viewerId, targetUserId, inherited.blockedBy, true, inherited.message) | |
| } else { | |
| addConfirmedBlock(viewerId, targetUserId, '', false) | |
| } | |
| return { | |
| isBlocked: Boolean(ownBlock) || inherited !== null, | |
| isOwnBlock: Boolean(ownBlock), | |
| inheritedFrom: inherited?.blockedBy ?? null | |
| const cachedOwnBlock = isInOwnBlocks(viewerId, targetUserId) | |
| const [ownBlock, followedBlockers] = await Promise.all([ | |
| this.getBlock(targetUserId, viewerId), | |
| this.getBlockFollows(viewerId) | |
| ]) | |
| const hasOwnBlock = cachedOwnBlock || ownBlock !== null | |
| const inherited = followedBlockers.length > 0 | |
| ? await this.checkInheritedBlocks(targetUserId, followedBlockers) | |
| : null | |
| if (hasOwnBlock) { | |
| addConfirmedBlock(viewerId, targetUserId, viewerId, true, ownBlock?.message) | |
| } else if (inherited) { | |
| addConfirmedBlock(viewerId, targetUserId, inherited.blockedBy, true, inherited.message) | |
| } else { | |
| addConfirmedBlock(viewerId, targetUserId, '', false) | |
| } | |
| return { | |
| isBlocked: hasOwnBlock || inherited !== null, | |
| isOwnBlock: hasOwnBlock, | |
| inheritedFrom: inherited?.blockedBy ?? null | |
| } |
source: ['codex']
| // Check if store owner is blocked | ||
| const { isBlocked: isOwnerBlocked, isOwnBlock } = useBlock(store?.ownerId ?? '') |
There was a problem hiding this comment.
🟡 Suggestion: Batch cart-owner block checks before rendering sections
Each cart store independently mounts useBlock. Without a confirmed result, the hook calls getBlockProvenance, which performs a direct-block query and then one query for every followed block list. A cart containing S stores while the viewer follows F block lists can therefore issue approximately S × (F + 1) Platform reads, even when the merged bloom filter proves that most owners are not blocked. Batch all cart owner IDs once with checkBlockedForAuthors in the cart page and pass the prefetched status into each section; checkBlockedBatch will seed the confirmed provenance cache consumed by these hooks.
source: ['codex']
3 participants
https://claude.ai/code/session_01Kry9GcgBhrpJuc8pR5CdcR
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.