Split bounty-detail-sidebar-cta into three focused files - #241
Conversation
|
@Biokes is attempting to deploy a commit to the Threadflow Team on Vercel. A member of the Team first needs to authorize it. |
|
@Biokes 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! 🚀 |
|
Warning Review limit reached
More reviews will be available in 27 minutes and 53 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughExtracts CTA state/handlers into a new client hook useBountyCTAState, moves MobileCTA to its own file, refactors SidebarCTA to consume the hook, and updates client imports to reference the separated MobileCTA component. ChangesBounty CTA State and Component Extraction
Sequence DiagramsequenceDiagram
participant Client as bounty-detail-client
participant SidebarCTA
participant MobileCTA
participant Hook as useBountyCTAState
participant Composed as ComposedHooks
Client->>SidebarCTA: render with bounty
Client->>MobileCTA: render with bounty
SidebarCTA->>Hook: useBountyCTAState({bounty, onCancelled})
MobileCTA->>Hook: useBountyCTAState({bounty, onCancelled})
Hook->>Composed: compose useApplyToBounty, useCompetitionJoinState, useCancelBountyDialog, useCanRaiseDispute
Composed-->>Hook: handlers + state
Hook-->>SidebarCTA: {walletAddress, handleApply, handleJoin, handleCancel, ctaLabel, flags...}
Hook-->>MobileCTA: {walletAddress, handleApply, handleJoin, handleCancel, ctaLabel, flags...}
SidebarCTA-->>Client: sidebar UI
MobileCTA-->>Client: mobile UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@components/bounty-detail/bounty-detail-mobile-cta.tsx`:
- Around line 77-87: The mobile CTA currently duplicates conditional label logic
(using hasJoined, canAct, isPastDeadline, bounty.status) and omits the explicit
"Cancelled" label; replace the inline ternary block in
bounty-detail-mobile-cta.tsx with a single call to the existing ctaLabel()
hook/function to ensure consistent labels across mobile/desktop and include
cancelled handling; update both occurrences (the one at the shown block and the
other at the later occurrence) to use ctaLabel() and remove the duplicated
ternary logic so the component reads its label only from ctaLabel().
- Around line 105-113: The icon-only cancel Buttons (the Button wrapping
<XCircle /> that calls setCancelDialogOpen(true)) lack accessible names; add an
explicit accessible label (e.g., aria-label="Cancel" or aria-label="Close") to
those Button components (both occurrences, including the other icon-only cancel
Button around lines 133-140) so assistive tech can announce them; update the
Button props to include aria-label (or aria-labelledby pointing to a
visually-hidden text node) on the Button elements containing only the XCircle
icon.
In `@components/bounty-detail/use-bounty-cta-state.ts`:
- Around line 65-67: Replace the hardcoded/ignored values so count fields from
the backend are used when present: set claimCount to use bounty.claimCount
first, falling back to bounty._count?.submissions then 0 (e.g., claimCount =
bounty.claimCount ?? bounty._count?.submissions ?? 0), and set maxParticipants
to read bounty.maxParticipants (e.g., maxParticipants: number | null =
bounty.maxParticipants ?? null) instead of forcing null; keep deadline as-is.
- Around line 72-79: The handleApply function currently returns early when
walletAddress is missing, causing the dialog to treat it as success; change it
to reject the submission by throwing an Error (or invoking the existing
toast/failure path) when walletAddress is falsy so the caller knows the apply
failed. Specifically, in handleApply (which calls applyToBounty with bounty.id
and walletAddress), replace the silent return with throwing a descriptive error
(or calling the toast failure helper) so the dialog stays open and shows the
failure state instead of closing silently.
🪄 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: 924c74e2-f473-4a47-b85d-380bd1f99423
📒 Files selected for processing (4)
components/bounty-detail/bounty-detail-client.tsxcomponents/bounty-detail/bounty-detail-mobile-cta.tsxcomponents/bounty-detail/bounty-detail-sidebar-cta.tsxcomponents/bounty-detail/use-bounty-cta-state.ts
Benjtalkshow
left a comment
There was a problem hiding this comment.
Good structure. The shared hook is well-organized and both components consume from it cleanly. This is the right split.
Three things to fix before merge.
TypeScript fails with 4 import errors. CancellationRecord is imported from @/types/bounty in all three files, but it actually lives at @/types/escrow. And ApplicationFormValues is imported from @/lib/graphql/generated in use-bounty-cta-state.ts, but it's exported from @/components/bounty/application-dialog. Fix the import paths and run pnpm tsc --noEmit locally before pushing.
The branch has merge conflicts against main. Please rebase.
SidebarCTA is 365 lines, over the 300-line target from the issue. The cancel dialog AlertDialog block is about 60 lines of JSX that could be extracted into a small CancelBountyDialog component to bring it under 300.
The merge made things worse, not better.
The original The merge then added 6 more errors inside the accidentally duplicated
Please:
The shared hook itself is well-structured. Once the merge cleanup is done correctly this will be ready. |
|
Latest comments fixed |
Benjtalkshow
left a comment
There was a problem hiding this comment.
Big turnaround. All the TS errors are gone, the duplicated MobileCTA is out of the sidebar file, the import paths are fixed, and the conflicts are resolved. Both components consume from the shared hook cleanly.
The sidebar file is at 366 lines, down from the original 680 but still over the 300-line target from the issue. Most of the remaining weight is the Cancel AlertDialog JSX (about 60 lines). I'm OK merging this as-is and opening a small follow-up to extract CancelBountyDialog into its own file, since the main duplication problem the issue was filed to solve is gone.
Merging this in. Thanks for sticking with it through the rounds.
Refactor: Split bounty-detail-sidebar-cta into three focused files
Problem
The
bounty-detail-sidebar-cta.tsxfile had grown to 680+ lines and exported bothSidebarCTAandMobileCTAcomponents from the same file. This created:handleApply,handleJoin,handleCanceletc. implemented twicehandleApplyfromSidebarCTAand broken the buildSolution
Split into three focused files:
use-bounty-cta-state.ts(new - ~115 lines)walletAddress,hasJoined,isPastDeadline, cancel/copy dialog statehandleJoin,handleApply,handleCancel,handleCopy,ctaLabelcanAct,isFcfs,isCompetition,isCreator,canRaiseDispute,canCancelclaimCount,deadline,isFinalized, etc.bounty-detail-sidebar-cta.tsx(refactored - ~250 lines)SidebarCTAuseBountyCTAStatehookbounty-detail-mobile-cta.tsx(new - ~150 lines)MobileCTAuseBountyCTAStatehookbounty-detail-client.tsx(updated)MobileCTAnow imported from./bounty-detail-mobile-ctaAcceptance Criteria ✅
Testing
Commits
Closes #209
Summary by CodeRabbit
New Features
Refactor