Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds frontend "Merge" and "Hide" dialog flows to the activist detail page, new API client methods and endpoint paths, and backend and model changes to require authenticated user chapter checks and to support a ChangesActivist Merge and Hide Features
sequenceDiagram
participant User as User
participant UI as Activist Detail UI
participant Dialog as Merge/Hide Dialog
participant API as API Client
participant Server as Server
participant DB as Database
participant Toast as Toast/Navigation
User->>UI: Click Merge or Hide action
UI->>Dialog: Open appropriate dialog
User->>Dialog: Provide target name (merge) or confirm (hide)
Dialog->>API: POST mergeActivist(...) or hideActivist(...)
API->>Server: HTTP POST /api/activist/merge or /api/activist/hide
Server->>DB: Begin transaction / fetch activist(s) / check chapter access
DB->>DB: Update/merge/hide records, commit
Server->>API: Return { status: "success" }
API->>Dialog: Resolve mutation
Dialog->>Dialog: Invalidate queries, close dialog
Dialog->>Toast: Show success toast
Toast->>User: Navigate to /activists
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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 (1)
server/src/model/activist.go (1)
1383-1390: ⚡ Quick winUse standard wrapping instead of adding new
pkg/errorscalls.These changed blocks introduce new
errors.Wrapf(...)usages. Please switch them tofmt.Errorf(...: %w)so new backend code stays on the standard library path.As per coding guidelines, `server/**/*.go`: Do not use `github.com/pkg/errors` for new code in the Go backend.♻️ Proposed fix
- return errors.Wrapf(err, "failed to get target activist with id %d", targetActivistID) + return fmt.Errorf("failed to get target activist with id %d: %w", targetActivistID, err)Also applies to: 1673-1676
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/model/activist.go` around lines 1383 - 1390, Replace the pkg/errors Wrapf calls with standard library wrapping using fmt.Errorf and %w in the blocks that fetch activists: where tx.Get is called for originalActivist (variable originalActivist, id originalActivistID) and targetActivist (variable targetActivist, id targetActivistID); keep the tx.Rollback() behavior but change the return values to use fmt.Errorf("failed to get original activist with id %d: %w", originalActivistID, err) and similarly for targetActivist, and update imports to remove github.com/pkg/errors and import "fmt" instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend-v2/src/app/`(authed)/activists/[id]/activist-detail.tsx:
- Around line 160-178: The Hide button is visible to all users but the backend
endpoint /activist/hide is protected by apiSFBayOrganizerAuthMiddleware; update
the UI to gate or disable the Hide action using the same access rule: check the
current user's organizer status (e.g., an isSFBayOrganizer flag from your
session/user context or hook) before rendering the Button that calls
setIsHideDialogOpen(true) (or render it disabled with a tooltip), so only users
who satisfy the apiSFBayOrganizerAuthMiddleware can see/activate the Hide
dialog.
In `@frontend-v2/src/app/`(authed)/activists/[id]/merge-activist-dialog.tsx:
- Around line 45-46: The merge dialog currently filters suggestions and blocks
selection by comparing names (activistName) and rejecting selectedValue ===
activistName, which prevents merging two different activists with the same
display name; change the flow to track and pass activist IDs instead of names:
update the suggestion items and selection state to hold targetActivistId (not
just name), modify the filter to exclude only the entry whose id equals
activistId (leave identical names allowed), and change mutationFn from
apiClient.mergeActivist(activistId, targetName) to call a merge API that accepts
the target ID (e.g., apiClient.mergeActivist(activistId, targetActivistId));
make the same adjustments in the other block referenced (lines ~70-76) so
selection, filtering, and the mutation use IDs rather than names.
In `@server/src/model/activist.go`:
- Around line 1378-1391: The plain snapshot reads using tx.Get with
selectActivistExtraBaseQuery do not lock rows; change the reads that populate
originalActivist and targetActivist to locking SELECTs (e.g., append " FOR
UPDATE" or use the DB's row-locking clause) so both activist rows are locked in
this transaction before computing mergedActivist and performing UPDATEs; update
the two tx.Get calls (the ones filling originalActivist and targetActivist from
selectActivistExtraBaseQuery) to use the locking query so the merge winner is
derived from stable, locked rows.
---
Nitpick comments:
In `@server/src/model/activist.go`:
- Around line 1383-1390: Replace the pkg/errors Wrapf calls with standard
library wrapping using fmt.Errorf and %w in the blocks that fetch activists:
where tx.Get is called for originalActivist (variable originalActivist, id
originalActivistID) and targetActivist (variable targetActivist, id
targetActivistID); keep the tx.Rollback() behavior but change the return values
to use fmt.Errorf("failed to get original activist with id %d: %w",
originalActivistID, err) and similarly for targetActivist, and update imports to
remove github.com/pkg/errors and import "fmt" instead.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d7d007fd-956b-46d3-8402-ec286c89b7ca
📒 Files selected for processing (7)
frontend-v2/src/app/(authed)/activists/[id]/activist-detail.tsxfrontend-v2/src/app/(authed)/activists/[id]/hide-activist-dialog.tsxfrontend-v2/src/app/(authed)/activists/[id]/merge-activist-dialog.tsxfrontend-v2/src/lib/api.tsserver/src/main.goserver/src/model/activist.goserver/src/model/activist_test.go
Allow name merges in the future for merging across chapters, but default to false because renaming activists based on timestamps when merging within a chapter from the UI may be a confusing UX.
alexsapps
force-pushed
the
alex/delete-merge-activists
branch
from
eec6eec to
be064bb
Compare
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 by CodeRabbit
New Features
Bug Fixes
Tests