Feature: Bounty Cancellation and Refund Flow - #170
Merged
Benjtalkshow merged 3 commits intoMar 30, 2026
Conversation
|
@codebestia is attempting to deploy a commit to the Threadflow Team on Vercel. A member of the Team first needs to authorize it. |
|
@codebestia Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughAdds a creator-facing bounty cancellation flow: UI to submit cancellation reason, EscrowService refund/cancellation operations and types, mutation hooks with optimistic updates, a RefundStatusTracker to show refund progress, and tests covering escrow cancellation/refund behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Creator
participant UI as SidebarCTA / MobileCTA
participant Dialog as Cancel Dialog (useCancelBountyDialog)
participant Escrow as EscrowService
participant GraphQL as useCancelBounty mutation
participant Client as BountyDetailClient
participant Tracker as RefundStatusTracker
User->>UI: Click "Cancel Bounty"
UI->>Dialog: open dialog / collect reason
User->>Dialog: Confirm with reason
Dialog->>Escrow: cancelBounty(bountyId, userId, reason)
Escrow-->>Dialog: CancellationRecord (includes RefundResult)
Dialog->>GraphQL: cancelAsync({ id, reason })
GraphQL-->>Dialog: Mutation resolves (optimistic status=CANCELLED)
Dialog->>Client: onCancelled(CancellationRecord)
Client->>Client: set local cancellationRecord -> isCancelled=true
Client->>Tracker: pass bountyId, isCancelled=true
Tracker->>Escrow: useCancellation -> getCancellation(bountyId)
Tracker-->>User: render refund status, tx hash, amount, explorer link
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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 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: 3
🧹 Nitpick comments (5)
hooks/__tests__/use-cancel-bounty.test.ts (1)
3-7: Tests are order-dependent due to shared static state.The comment acknowledges that tests have side effects on each other, but relies on "careful ordering" which is fragile. If tests are run in isolation or if the runner configuration changes, these tests will fail unpredictably.
Consider adding a reset method to
EscrowServiceand calling it inbeforeEachto ensure test isolation.🧪 Proposed fix: Add reset capability
In
lib/services/escrow.ts, add:// For testing only - reset to initial state static __resetForTesting(): void { this.pools = { /* initial pool data */ }; this.cancellations = {}; }Then in the test file:
describe("EscrowService - Cancellation & Refund", () => { - // Reset the static mock state between tests by re-importing - // EscrowService is a static singleton, so tests may have side effects - // on each other for the cancel/refund methods. We test in a careful order. + beforeEach(() => { + EscrowService.__resetForTesting(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/__tests__/use-cancel-bounty.test.ts` around lines 3 - 7, Tests rely on shared static state in EscrowService which makes them order-dependent; add a test-only static reset method on the EscrowService class (e.g., EscrowService.__resetForTesting) that restores internal static properties like pools and cancellations to their initial values, and call that reset in the test suite's beforeEach (in use-cancel-bounty.test.ts) to ensure each test starts from a clean EscrowService state.components/bounty-detail/bounty-detail-sidebar-cta.tsx (1)
302-330: Duplicate cancellation logic in MobileCTA.The
handleCancelfunction inMobileCTAduplicates the logic fromSidebarCTA. This increases maintenance burden and introduces the same atomicity issue flagged above.♻️ Extract shared hook or helper
Consider extracting the cancellation logic into a shared hook:
function useBountyCancellation( bountyId: string, onCancelled?: (record: CancellationRecord) => void ) { const [cancelDialogOpen, setCancelDialogOpen] = useState(false); const [cancelReason, setCancelReason] = useState(""); const [isCancelling, setIsCancelling] = useState(false); const { data: session } = authClient.useSession(); const cancelBounty = useCancelBounty(); const handleCancel = async () => { // ... shared logic }; return { cancelDialogOpen, setCancelDialogOpen, cancelReason, setCancelReason, isCancelling, handleCancel, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/bounty-detail/bounty-detail-sidebar-cta.tsx` around lines 302 - 330, The MobileCTA and SidebarCTA both implement nearly identical handleCancel logic; extract this into a shared hook (e.g., useBountyCancellation) that encapsulates cancelDialogOpen, setCancelDialogOpen, cancelReason, setCancelReason, isCancelling, setIsCancelling and the handleCancel procedure which calls EscrowService.cancelBounty, invokes cancelBountyMutation.cancel, shows toasts, and calls onCancelled; update both MobileCTA and SidebarCTA to use the hook and wire their local UI controls to the returned state and handleCancel so the cancellation logic is single-sourced and avoids duplication.components/bounty/refund-status.tsx (1)
117-125: Consider adding error feedback for clipboard failures.The empty catch block silently swallows clipboard errors. While this is often acceptable for clipboard operations, users might appreciate feedback when copy fails (e.g., on browsers without clipboard permissions).
📋 Optional: Add toast notification on failure
const handleCopyHash = async () => { try { await navigator.clipboard.writeText(refund.transactionHash); setCopiedHash(true); setTimeout(() => setCopiedHash(false), 2000); } catch { - // clipboard write failed + // Optionally notify user - clipboard may be unavailable + // toast.error("Failed to copy to clipboard"); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/bounty/refund-status.tsx` around lines 117 - 125, The catch block in handleCopyHash silently swallows clipboard errors; update it to surface failures by logging the caught error and showing user feedback (e.g., call a toast utility or set an error state) so users know the copy failed. Specifically, inside handleCopyHash (which calls navigator.clipboard.writeText and updates setCopiedHash), catch the error as e, call console.error or processLogger with e and display a toast/notification like "Copy failed" (or set an error state to render a message), and ensure setCopiedHash is not left true on failure.hooks/use-bounty-mutations.ts (2)
244-252: JSDoc is inaccurate –refundResultis not returned.The JSDoc mentions
refundResultin the return type, but the hook doesn't track or return any refund result. The escrow cancellation is handled separately in the CTA component viaEscrowService.cancelBounty().📝 Proposed documentation fix
/** * Hook to cancel a bounty and trigger escrow refund - * Calls EscrowService.cancelBounty which simulates on-chain - * BountyRegistry.cancel_bounty() + CoreEscrow.refund_all() + * Updates bounty status to CANCELLED via GraphQL mutation. + * Note: Escrow refund is triggered separately via EscrowService.cancelBounty() + * in the UI layer before calling this mutation. * - * `@returns` Mutation object with cancel method and refund result state + * `@returns` Mutation object with cancel/cancelAsync methods * `@example` - * const { cancel, isPending, refundResult } = useCancelBounty(); - * cancel({ bountyId: "123", reason: "No longer needed" }); + * const { cancel, isPending } = useCancelBounty(); + * cancel({ id: "123", reason: "No longer needed" }); */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/use-bounty-mutations.ts` around lines 244 - 252, The JSDoc for the bounty-cancel hook is inaccurate because it claims a refundResult is returned though the hook does not track or return any refund result; update the comment on the hook (e.g., the JSDoc above useCancelBounty / useCancelBountyMutation) to remove references to refundResult, adjust the `@returns` description to list only the actual returned properties (e.g., cancel and isPending), and fix the example to stop destructuring refundResult; also mention that escrow cancellation is handled separately via EscrowService.cancelBounty if you want to keep a note about refunds.
296-311: Thereasonparameter is accepted but silently ignored.Both
cancel()andcancelAsync()acceptreasonin their signature, but it's never included in the mutation input. TheUpdateBountyInputGraphQL type doesn't have areasonfield, so this is intentional—but the current API is misleading.Consider either removing
reasonfrom these signatures (since it's only used inEscrowService.cancelBounty()) or adding a comment explaining this.♻️ Option 1: Remove unused parameter
cancel: ( - { id, reason }: { id: string; reason?: string }, + { id }: { id: string }, options?: UpdateBountyMutateOptions, ) => mutation.mutate( { input: { id, status: "CANCELLED" } }, options, ), cancelAsync: ( - { id, reason }: { id: string; reason?: string }, + { id }: { id: string }, options?: UpdateBountyMutateOptions, ) => mutation.mutateAsync( { input: { id, status: "CANCELLED" } }, options, ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/use-bounty-mutations.ts` around lines 296 - 311, The cancel and cancelAsync functions currently accept a reason parameter but never send it in the mutation input (mutation.mutate / mutation.mutateAsync) because UpdateBountyInput has no reason field; update the signatures in cancel and cancelAsync to remove the unused { reason } parameter (keep only { id }) or, if you prefer to keep the parameter for API parity, add a clear inline comment in hooks/use-bounty-mutations.ts next to cancel and cancelAsync explaining that reason is intentionally ignored here and is only consumed by EscrowService.cancelBounty; ensure references to UpdateBountyInput and EscrowService.cancelBounty are mentioned in the comment so future readers understand why reason is omitted from the mutation payload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/bounty-detail/bounty-detail-client.tsx`:
- Around line 80-93: The UI can get stuck showing "Refund is being processed"
when bounty.status === "CANCELLED" but cancellationRecord is null (e.g.,
in-memory mock reset); update the component logic around isCancelled and the
RefundStatusTracker/EscrowDetailPanel usage to handle a missing cancellation
record: detect the case where bounty.status === "CANCELLED" &&
cancellationRecord === null and pass a prop (or alternate flag) to
RefundStatusTracker (or render a fallback message) indicating "Refund details
unavailable — check Stellar explorer" (or a TODO pointing to `#139`) instead of
the in-progress state; ensure the change references the existing symbols
isCancelled, cancellationRecord, RefundStatusTracker, and
EscrowService.getCancellation so reviewers can locate and adjust the
rendering/fallback behavior.
In `@components/bounty-detail/bounty-detail-sidebar-cta.tsx`:
- Around line 69-95: The cancellation is non-atomic: you call
EscrowService.cancelBounty(...) then fire cancelBounty.cancel(...) without
awaiting, so if the GraphQL mutation fails the escrow is already mutated. Fix by
making the flow atomic: either (A) await the GraphQL mutation first (await
cancelBounty.cancel({...})) and only call await EscrowService.cancelBounty(...)
after the mutation succeeds, or (B) if you must call escrow first, await
EscrowService.cancelBounty(...) then await cancelBounty.cancel(...) and on
GraphQL failure call a rollback on the escrow (implement and call a revert
method like EscrowService.revertCancel(bounty.id, session?.user?.id ?? "",
record.id)) and surface the combined error. Update the async handler around
EscrowService.cancelBounty, cancelBounty.cancel, onCancelled and toast so all
calls are awaited and setIsCancelling is cleared in finally.
In `@lib/services/escrow.ts`:
- Around line 206-214: Currently the refund branch resets releasedAmount to 0
and sets refundedAmount = pool.totalAmount, which erases prior release history
and overstates the refund; instead compute the actual refundable amount as
(pool.totalAmount - pool.releasedAmount), set refundedAmount to that value, do
not modify pool.releasedAmount (preserve history), and update this.pools[poolId]
only to set isLocked: false and status: "Refunded" (keeping releasedAmount and
totalAmount intact); locate the logic around generateMockTxHash, refundedAmount,
and this.pools[poolId] to implement this change.
---
Nitpick comments:
In `@components/bounty-detail/bounty-detail-sidebar-cta.tsx`:
- Around line 302-330: The MobileCTA and SidebarCTA both implement nearly
identical handleCancel logic; extract this into a shared hook (e.g.,
useBountyCancellation) that encapsulates cancelDialogOpen, setCancelDialogOpen,
cancelReason, setCancelReason, isCancelling, setIsCancelling and the
handleCancel procedure which calls EscrowService.cancelBounty, invokes
cancelBountyMutation.cancel, shows toasts, and calls onCancelled; update both
MobileCTA and SidebarCTA to use the hook and wire their local UI controls to the
returned state and handleCancel so the cancellation logic is single-sourced and
avoids duplication.
In `@components/bounty/refund-status.tsx`:
- Around line 117-125: The catch block in handleCopyHash silently swallows
clipboard errors; update it to surface failures by logging the caught error and
showing user feedback (e.g., call a toast utility or set an error state) so
users know the copy failed. Specifically, inside handleCopyHash (which calls
navigator.clipboard.writeText and updates setCopiedHash), catch the error as e,
call console.error or processLogger with e and display a toast/notification like
"Copy failed" (or set an error state to render a message), and ensure
setCopiedHash is not left true on failure.
In `@hooks/__tests__/use-cancel-bounty.test.ts`:
- Around line 3-7: Tests rely on shared static state in EscrowService which
makes them order-dependent; add a test-only static reset method on the
EscrowService class (e.g., EscrowService.__resetForTesting) that restores
internal static properties like pools and cancellations to their initial values,
and call that reset in the test suite's beforeEach (in
use-cancel-bounty.test.ts) to ensure each test starts from a clean EscrowService
state.
In `@hooks/use-bounty-mutations.ts`:
- Around line 244-252: The JSDoc for the bounty-cancel hook is inaccurate
because it claims a refundResult is returned though the hook does not track or
return any refund result; update the comment on the hook (e.g., the JSDoc above
useCancelBounty / useCancelBountyMutation) to remove references to refundResult,
adjust the `@returns` description to list only the actual returned properties
(e.g., cancel and isPending), and fix the example to stop destructuring
refundResult; also mention that escrow cancellation is handled separately via
EscrowService.cancelBounty if you want to keep a note about refunds.
- Around line 296-311: The cancel and cancelAsync functions currently accept a
reason parameter but never send it in the mutation input (mutation.mutate /
mutation.mutateAsync) because UpdateBountyInput has no reason field; update the
signatures in cancel and cancelAsync to remove the unused { reason } parameter
(keep only { id }) or, if you prefer to keep the parameter for API parity, add a
clear inline comment in hooks/use-bounty-mutations.ts next to cancel and
cancelAsync explaining that reason is intentionally ignored here and is only
consumed by EscrowService.cancelBounty; ensure references to UpdateBountyInput
and EscrowService.cancelBounty are mentioned in the comment so future readers
understand why reason is omitted from the mutation payload.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c146ac6-8961-46d4-bda9-7c4f3de2e9a7
📒 Files selected for processing (8)
components/bounty-detail/bounty-detail-client.tsxcomponents/bounty-detail/bounty-detail-sidebar-cta.tsxcomponents/bounty/refund-status.tsxhooks/__tests__/use-cancel-bounty.test.tshooks/use-bounty-mutations.tshooks/use-escrow.tslib/services/escrow.tstypes/escrow.ts
Benjtalkshow
requested changes
Mar 30, 2026
Contributor
There was a problem hiding this comment.
Hi @codebestia , great work on the cancellation and refund flow. The AlertDialog confirmation UX, RefundStatusTracker with copy-to-clipboard, and the escrow service structure are all well-built and the types are clean!
The critical issue is that cancelBounty.cancel() in both SidebarCTA and MobileCTA calls mutation.mutate() (fire-and-forget) instead of awaiting cancelAsync(). This means the success toast and onCancelled callback fire immediately after the escrow call, regardless of whether the GraphQL status update succeeded. If it fails, the escrow is already marked "Refunded" while the bounty remains unchanged. The reason parameter is also accepted by .cancel() but never sent to the GraphQL mutation ({ input: { id, status: "CANCELLED" } } ignores it), so cancellation reasons are silently lost.
Please address the following:
refundAll() resets releasedAmount: 0, which erases release history for partially released pools. It should only refund totalAmount - releasedAmount.
Add a fallback message in RefundStatusTracker when isCancelled is true but no cancellation record exists, since a page reload loses the in-memory mock data.
Extract the duplicated cancel dialog logic from SidebarCTA and MobileCTA into a shared hook.
Address coderabbit's corrections too.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
hooks/use-bounty-mutations.ts (1)
300-313: Suppress lint warning for intentionally unusedreasonparameter.The static analysis flags
reasonas unused. Since the comments explain it's intentionally not passed to GraphQL, use underscore prefix to suppress the warning.♻️ Proposed fix
cancel: ( - { id, reason }: { id: string; reason?: string }, + { id, reason: _reason }: { id: string; reason?: string }, options?: UpdateBountyMutateOptions, ) => // 'reason' is intentionally ignored in the GraphQL mutation because // UpdateBountyInput does not support it. It is consumed by EscrowService.cancelBounty. mutation.mutate({ input: { id, status: "CANCELLED" } }, options), cancelAsync: ( - { id, reason }: { id: string; reason?: string }, + { id, reason: _reason }: { id: string; reason?: string }, options?: UpdateBountyMutateOptions, ) => // 'reason' is intentionally ignored in the GraphQL mutation because // UpdateBountyInput does not support it. It is consumed by EscrowService.cancelBounty. mutation.mutateAsync({ input: { id, status: "CANCELLED" } }, options),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/use-bounty-mutations.ts` around lines 300 - 313, Rename the intentionally unused parameter `reason` to `_reason` (or `reason_`) in both `cancel` and `cancelAsync` arrow functions inside hooks/use-bounty-mutations (the destructured param `{ id, reason }: { id: string; reason?: string }`) to suppress the lint warning; keep the existing comment about why it's ignored and leave the mutation calls (`mutation.mutate` and `mutation.mutateAsync`) unchanged so the GraphQL input remains `{ id, status: "CANCELLED" }`.components/bounty-detail/bounty-detail-sidebar-cta.tsx (1)
25-25: Remove unusedtoastimport.The static analysis correctly flags that
toastis imported but never used in this component. Toast calls are handled withinuseCancelBountyDialog.♻️ Proposed fix
-import { toast } from "sonner";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/bounty-detail/bounty-detail-sidebar-cta.tsx` at line 25, Remove the unused import of toast from the component: delete the line importing toast (import { toast } from "sonner";) in bounty-detail-sidebar-cta.tsx since all toast calls are handled inside useCancelBountyDialog; ensure no other references to the toast symbol remain in the file and run the build/linter to confirm the unused-import warning is resolved.lib/services/escrow.ts (2)
228-247:revertCancelinfers status instead of storing original state.The
CancellationRecordtype (pertypes/escrow.ts:45-51) doesn't store the original pool status before cancellation. TherevertCancelmethod infers the status fromreleasedAmount, which works correctly for the current "Escrowed" and "Partially Released" states but is inherently lossy.For the current mock implementation this is acceptable, but when integrating with real contract bindings (
#139), consider storing the original status inCancellationRecordfor accurate restoration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/escrow.ts` around lines 228 - 247, revertCancel currently infers the pool status from releasedAmount which is lossy; update the CancellationRecord type to add a field (e.g., originalStatus: string) and when creating a cancellation record store the pool's current status into originalStatus, then change revertCancel (in lib/services/escrow.ts) to read originalStatus from this.cancellations[bountyId] and restore that exact status on this.pools[bountyId] instead of recomputing it from releasedAmount; ensure any code paths that write to this.cancellations (the cancellation creator) are updated to set originalStatus.
83-115: Test reset duplicates pool initialization data.The
__resetForTestingmethod duplicates the initial pool configuration from lines 11-42. This creates a maintenance burden where any changes to the initial mock data must be synchronized in two places.Consider extracting the initial state into a constant:
♻️ Proposed refactor to reduce duplication
+const INITIAL_POOLS: Record<string, EscrowPool> = { + "1": { + poolId: "1", + totalAmount: 500, + asset: "USDC", + isLocked: true, + expiry: new Date(Date.now() + 30 * 24 * 60 * 60 * 1000).toISOString(), + releasedAmount: 0, + status: "Escrowed", + }, + // ... other pools +}; export class EscrowService { - private static pools: Record<string, EscrowPool> = { - "1": { ... }, - ... - }; + private static pools: Record<string, EscrowPool> = { ...INITIAL_POOLS }; static __resetForTesting() { - this.pools = { ... }; // duplicated + this.pools = structuredClone(INITIAL_POOLS); this.cancellations = {}; }Note: For a mock service that will be replaced by real contract bindings (
#139), this duplication is acceptable to defer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/escrow.ts` around lines 83 - 115, The __resetForTesting method duplicates the initial mock pool data; extract the shared initial pool state into a single constant (e.g. INITIAL_POOLS or DEFAULT_POOLS) and have both the class' initial this.pools initialization and the static __resetForTesting method reference/clone that constant (ensuring you clone to avoid shared mutation), and leave this.cancellations handling as-is; update references to use the constant in Escrow service so future changes are made in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/bounty-detail/bounty-detail-sidebar-cta.tsx`:
- Around line 229-234: The desktop AlertDialogCancel currently calls
setCancelReason("") on click (clearing the input) but the mobile dialog's
AlertDialogCancel in MobileCTA does not, causing stale text when reopened;
update the mobile AlertDialogCancel (the one rendered in MobileCTA) to call
setCancelReason("") in its onClick handler (matching the desktop behavior) so
both dialogs clear the cancel reason consistently when dismissed.
In `@hooks/use-bounty-mutations.ts`:
- Around line 290-295: The onSettled handler currently invalidates only
bountyKeys.detail(variables.input.id) and bountyKeys.lists(), which misses other
list variants (ActiveBounties, OrganizationBounties, ProjectBounties); update
the invalidation to use bountyKeys.allListKeys instead of bountyKeys.lists() and
keep the detail invalidation (i.e., in the onSettled callback where
queryClient.invalidateQueries is called, replace the second invalidateQueries
call to reference bountyKeys.allListKeys to comprehensively refresh all bounty
list queries).
In `@hooks/use-cancel-bounty-dialog.ts`:
- Around line 30-34: The hook useCancelBountyDialog calls
EscrowService.cancelBounty passing session?.user?.id ?? "" which can send an
empty user id; add an explicit guard in the hook to validate the authenticated
user before calling cancelBounty (e.g., compute const userId = session?.user?.id
and if (!userId) return/throw an error), so the function bails out or surfaces
an error when no authenticated user is present even if UI-level canCancel
(isCreator) should prevent it; update any error handling path to log or surface
a clear message and keep the call to EscrowService.cancelBounty only when userId
is defined.
---
Nitpick comments:
In `@components/bounty-detail/bounty-detail-sidebar-cta.tsx`:
- Line 25: Remove the unused import of toast from the component: delete the line
importing toast (import { toast } from "sonner";) in
bounty-detail-sidebar-cta.tsx since all toast calls are handled inside
useCancelBountyDialog; ensure no other references to the toast symbol remain in
the file and run the build/linter to confirm the unused-import warning is
resolved.
In `@hooks/use-bounty-mutations.ts`:
- Around line 300-313: Rename the intentionally unused parameter `reason` to
`_reason` (or `reason_`) in both `cancel` and `cancelAsync` arrow functions
inside hooks/use-bounty-mutations (the destructured param `{ id, reason }: { id:
string; reason?: string }`) to suppress the lint warning; keep the existing
comment about why it's ignored and leave the mutation calls (`mutation.mutate`
and `mutation.mutateAsync`) unchanged so the GraphQL input remains `{ id,
status: "CANCELLED" }`.
In `@lib/services/escrow.ts`:
- Around line 228-247: revertCancel currently infers the pool status from
releasedAmount which is lossy; update the CancellationRecord type to add a field
(e.g., originalStatus: string) and when creating a cancellation record store the
pool's current status into originalStatus, then change revertCancel (in
lib/services/escrow.ts) to read originalStatus from this.cancellations[bountyId]
and restore that exact status on this.pools[bountyId] instead of recomputing it
from releasedAmount; ensure any code paths that write to this.cancellations (the
cancellation creator) are updated to set originalStatus.
- Around line 83-115: The __resetForTesting method duplicates the initial mock
pool data; extract the shared initial pool state into a single constant (e.g.
INITIAL_POOLS or DEFAULT_POOLS) and have both the class' initial this.pools
initialization and the static __resetForTesting method reference/clone that
constant (ensuring you clone to avoid shared mutation), and leave
this.cancellations handling as-is; update references to use the constant in
Escrow service so future changes are made in one place.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 48bc6d0e-3281-45ca-afce-d4ad56b657b4
📒 Files selected for processing (7)
components/bounty-detail/bounty-detail-client.tsxcomponents/bounty-detail/bounty-detail-sidebar-cta.tsxcomponents/bounty/refund-status.tsxhooks/__tests__/use-cancel-bounty.test.tshooks/use-bounty-mutations.tshooks/use-cancel-bounty-dialog.tslib/services/escrow.ts
✅ Files skipped from review due to trivial changes (1)
- hooks/tests/use-cancel-bounty.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- components/bounty/refund-status.tsx
- components/bounty-detail/bounty-detail-client.tsx
Comment on lines
+229
to
+234
| <AlertDialogCancel | ||
| disabled={isCancelling} | ||
| onClick={() => setCancelReason("")} | ||
| > | ||
| Keep Bounty | ||
| </AlertDialogCancel> |
There was a problem hiding this comment.
Inconsistent dialog cancel behavior between desktop and mobile.
The desktop AlertDialogCancel (line 231) clears the reason on click, but the mobile version (line 341) does not. This creates inconsistent UX where reopening the mobile dialog may show stale text.
🔧 Proposed fix for MobileCTA
- <AlertDialogCancel disabled={isCancelling}>
+ <AlertDialogCancel
+ disabled={isCancelling}
+ onClick={() => setCancelReason("")}
+ >
Keep Bounty
</AlertDialogCancel>Also applies to: 340-343
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/bounty-detail/bounty-detail-sidebar-cta.tsx` around lines 229 -
234, The desktop AlertDialogCancel currently calls setCancelReason("") on click
(clearing the input) but the mobile dialog's AlertDialogCancel in MobileCTA does
not, causing stale text when reopened; update the mobile AlertDialogCancel (the
one rendered in MobileCTA) to call setCancelReason("") in its onClick handler
(matching the desktop behavior) so both dialogs clear the cancel reason
consistently when dismissed.
Comment on lines
+290
to
+295
| onSettled: (_data, _err, variables) => { | ||
| queryClient.invalidateQueries({ | ||
| queryKey: bountyKeys.detail(variables.input.id), | ||
| }); | ||
| queryClient.invalidateQueries({ queryKey: bountyKeys.lists() }); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if other mutation hooks have the same narrow invalidation pattern
rg -n "bountyKeys\.lists\(\)" --type=ts -C2Repository: boundlessfi/bounties
Length of output: 3638
🏁 Script executed:
find . -name "query-keys.ts" -type fRepository: boundlessfi/bounties
Length of output: 89
🏁 Script executed:
head -80 lib/query/query-keys.tsRepository: boundlessfi/bounties
Length of output: 2685
🏁 Script executed:
rg -n "allListKeys|ActiveBounties|OrganizationBounties|ProjectBounties" lib/query/query-keys.tsRepository: boundlessfi/bounties
Length of output: 894
🏁 Script executed:
rg -A5 "bountyKeys\.lists\(\)" hooks/use-bounty-mutations.ts | grep -A5 "onSettled\|onSuccess\|onMutate"Repository: boundlessfi/bounties
Length of output: 46
🏁 Script executed:
# Check if allListKeys is used anywhere else in codebase for invalidation patterns
rg -n "allListKeys" --type=tsRepository: boundlessfi/bounties
Length of output: 482
Cache invalidation may miss other bounty list queries.
Per lib/query/query-keys.ts, bountyKeys.lists() only covers the base ["Bounties"] query key. Cancelled bounties may still appear in ActiveBounties, OrganizationBounties, or ProjectBounties views until they're individually refreshed.
Use bountyKeys.allListKeys for comprehensive invalidation (a pattern already established in use-bounty-subscription.ts):
🔧 Proposed fix
onSettled: (_data, _err, variables) => {
queryClient.invalidateQueries({
queryKey: bountyKeys.detail(variables.input.id),
});
- queryClient.invalidateQueries({ queryKey: bountyKeys.lists() });
+ bountyKeys.allListKeys.forEach((key) => {
+ queryClient.invalidateQueries({ queryKey: key });
+ });
},Note: This pattern is repeated across multiple bounty and submission mutations in the codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hooks/use-bounty-mutations.ts` around lines 290 - 295, The onSettled handler
currently invalidates only bountyKeys.detail(variables.input.id) and
bountyKeys.lists(), which misses other list variants (ActiveBounties,
OrganizationBounties, ProjectBounties); update the invalidation to use
bountyKeys.allListKeys instead of bountyKeys.lists() and keep the detail
invalidation (i.e., in the onSettled callback where
queryClient.invalidateQueries is called, replace the second invalidateQueries
call to reference bountyKeys.allListKeys to comprehensively refresh all bounty
list queries).
Comment on lines
+30
to
+34
| record = await EscrowService.cancelBounty( | ||
| bountyId, | ||
| session?.user?.id ?? "", | ||
| cancelReason.trim(), | ||
| ); |
There was a problem hiding this comment.
Missing validation for authenticated user.
If session?.user?.id is undefined, an empty string is passed to EscrowService.cancelBounty. While the UI guards this via canCancel (requires isCreator), the hook itself doesn't validate, which could create invalid cancellation records if called incorrectly.
Consider adding an explicit guard:
🛡️ Proposed defensive check
const handleCancel = async () => {
if (!cancelReason.trim()) {
toast.error("Please provide a reason for cancellation");
return;
}
+ const userId = session?.user?.id;
+ if (!userId) {
+ toast.error("You must be logged in to cancel a bounty");
+ return;
+ }
+
setIsCancelling(true);
let record: CancellationRecord | null = null;
try {
// 1. Trigger escrow refund first (simulates on-chain call)
record = await EscrowService.cancelBounty(
bountyId,
- session?.user?.id ?? "",
+ userId,
cancelReason.trim(),
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hooks/use-cancel-bounty-dialog.ts` around lines 30 - 34, The hook
useCancelBountyDialog calls EscrowService.cancelBounty passing session?.user?.id
?? "" which can send an empty user id; add an explicit guard in the hook to
validate the authenticated user before calling cancelBounty (e.g., compute const
userId = session?.user?.id and if (!userId) return/throw an error), so the
function bails out or surfaces an error when no authenticated user is present
even if UI-level canCancel (isCreator) should prevent it; update any error
handling path to log or surface a clear message and keep the call to
EscrowService.cancelBounty only when userId is defined.
This was referenced May 26, 2026
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
2 participants
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
Implements the full bounty cancellation and escrow refund flow, allowing bounty creators to cancel open/in-progress bounties with on-chain refund via the escrow contract. Depends on #139 (TypeScript contract bindings) for production deployment — currently wired to mock EscrowService.
Changes
Types (types/escrow.ts)
Service Layer (lib/services/escrow.ts)
BountyRegistry.cancel_bounty(creator, bounty_id)+ automatic refundCoreEscrow.refund_all(pool_id)CoreEscrow.refund_remaining(pool_id)Hooks
Components
Acceptance Criteria
Related Issues
Closes #149
Summary by CodeRabbit
New Features
Tests