admin permissions - #338
Merged
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 43 minutes and 35 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR implements a complete activity RSVP removal feature with refined membership authorization. It adds a new DELETE endpoint for removing user RSVPs from activities, requires admin status to remove trip members, prevents removal of a trip's last admin, and gates editing UI based on user permissions in the activity and housing detail screens. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/docs/docs.go`:
- Line 3191: The API docs string "Removes a user from a trip. Only admins can
remove members." is incorrect; update the endpoint's annotation/comment (the
description string shown in docs.go for the trip membership removal endpoint) to
reflect that admins can remove other members and users may remove themselves
(e.g., "Removes a user from a trip. Admins may remove other members; users may
remove themselves."), then regenerate the Swagger/OpenAPI spec so the contract
matches runtime behavior.
In `@backend/docs/swagger.json`:
- Line 3185: Update the operation's "description" for the membership removal
endpoint to reflect that both admins and the user themself can remove a
membership (not "Only admins"), e.g. change the text in the "description"
property associated with the remove-member operation so it states that admins
can remove members and individual users may remove their own membership; ensure
the revised description is applied to the same remove-user endpoint's
"description" field in swagger.json so clients and tests read the corrected
contract.
In `@backend/docs/swagger.yaml`:
- Line 4011: Update the OpenAPI description string for the "Removes a user from
a trip" operation to reflect that removal is allowed by either an admin or the
user themself (self-removal), not only admins; modify the description value
currently reading "Removes a user from a trip. Only admins can remove members."
to a clearer sentence such as "Removes a user from a trip. This action may be
performed by an admin or by the user themself (self-removal)." so API consumers
understand permitted callers.
In `@backend/internal/services/activity.go`:
- Around line 386-407: The authorization in ActivityService.RemoveActivityRSVP
incorrectly blocks callers who are removing their own RSVP because the check
only allows admins or the activity proposer; change the logic to also permit
self-removal by allowing the operation when callerID == targetUserID (or
explicitly enforce denial if intended), i.e., update the condition that
currently uses isAdmin and isProposer to also check targetUserID == callerID
before returning errs.Forbidden; reference RemoveActivityRSVP, isAdmin,
isProposer, callerID, targetUserID and ensure this behavior aligns with
UpdateActivityRSVP (which only upserts) and the intended UX (or add a comment
documenting the deliberate denial if you choose not to allow self-removal).
In `@backend/internal/tests/activity_test.go`:
- Around line 1805-1816: Rename the subtest title for clarity: change the t.Run
invocation currently named "non-member cannot remove an RSVP" to something
explicit like "non-member receives 404 (trip not visible)" so the test intent
(authorization/visibility causing 404) is unambiguous and cannot be confused
with the separate "removing an RSVP that does not exist returns 404" case;
update the t.Run call surrounding the Request that targets
fmt.Sprintf("/api/v1/trips/%s/activities/%s/rsvps/%s", trip, activityID, member)
with UserID set to &nonMember and AssertStatus(http.StatusNotFound) to use the
new descriptive name.
- Around line 1742-1753: The rsvpUser helper is using testkit.POST but the RSVP
route expects PUT; update the helper function (rsvpUser) to use testkit.PUT
instead of testkit.POST when calling Request for the
"/api/v1/trips/%s/activities/%s/rsvps" route so the request matches the
registered route and the AssertStatus(http.StatusOK) checks succeed; keep the
same Request body (models.ActivityRSVPRequestPayload{Status:
models.RSVPStatusGoing}) and other parameters unchanged.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 6f4acfb4-d01d-4dc9-be6d-5d176ff4d9c7
📒 Files selected for processing (12)
backend/docs/docs.gobackend/docs/swagger.jsonbackend/docs/swagger.yamlbackend/internal/controllers/activity.gobackend/internal/controllers/membership.gobackend/internal/repository/activity.gobackend/internal/repository/activity_rsvp.gobackend/internal/server/routers/activity.gobackend/internal/services/activity.gobackend/internal/services/membership.gobackend/internal/tests/activity_test.gobackend/internal/tests/membership_test.go
| }, | ||
| "delete": { | ||
| "description": "Removes a user from a trip", | ||
| "description": "Removes a user from a trip. Only admins can remove members.", |
Contributor
There was a problem hiding this comment.
Authorization description is incorrect for membership removal.
Line 3191 says only admins can remove members, but this endpoint also allows users to remove themselves. Please update the annotation/source comment and regenerate Swagger so the contract matches runtime behavior.
Suggested documentation fix
- "description": "Removes a user from a trip. Only admins can remove members.",
+ "description": "Removes a user from a trip. Trip admins can remove any member, and users can remove themselves.",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/docs/docs.go` at line 3191, The API docs string "Removes a user from
a trip. Only admins can remove members." is incorrect; update the endpoint's
annotation/comment (the description string shown in docs.go for the trip
membership removal endpoint) to reflect that admins can remove other members and
users may remove themselves (e.g., "Removes a user from a trip. Admins may
remove other members; users may remove themselves."), then regenerate the
Swagger/OpenAPI spec so the contract matches runtime behavior.
| }, | ||
| "delete": { | ||
| "description": "Removes a user from a trip", | ||
| "description": "Removes a user from a trip. Only admins can remove members.", |
Contributor
There was a problem hiding this comment.
Authorization description is incorrect for membership removal.
Line 3185 states that only admins can remove members, but this endpoint also allows users to remove themselves. This creates an inaccurate API contract for clients and test expectations.
Proposed doc fix
- "description": "Removes a user from a trip. Only admins can remove members.",
+ "description": "Removes a user from a trip. Caller must be a trip admin or the target user themself.",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/docs/swagger.json` at line 3185, Update the operation's "description"
for the membership removal endpoint to reflect that both admins and the user
themself can remove a membership (not "Only admins"), e.g. change the text in
the "description" property associated with the remove-member operation so it
states that admins can remove members and individual users may remove their own
membership; ensure the revised description is applied to the same remove-user
endpoint's "description" field in swagger.json so clients and tests read the
corrected contract.
| /api/v1/trips/{tripID}/memberships/{userID}: | ||
| delete: | ||
| description: Removes a user from a trip | ||
| description: Removes a user from a trip. Only admins can remove members. |
Contributor
There was a problem hiding this comment.
Description is incomplete: self-removal is also permitted.
Per the PR objectives, the caller may be an admin or the user themself. The current text ("Only admins can remove members") would mislead API consumers into thinking a member cannot leave a trip via this endpoint.
Proposed wording
- description: Removes a user from a trip. Only admins can remove members.
+ description: Removes a user from a trip. Only a trip admin or the user
+ themself can perform this action. The last remaining admin cannot be
+ removed.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| description: Removes a user from a trip. Only admins can remove members. | |
| description: Removes a user from a trip. Only a trip admin or the user | |
| themself can perform this action. The last remaining admin cannot be | |
| removed. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/docs/swagger.yaml` at line 4011, Update the OpenAPI description
string for the "Removes a user from a trip" operation to reflect that removal is
allowed by either an admin or the user themself (self-removal), not only admins;
modify the description value currently reading "Removes a user from a trip. Only
admins can remove members." to a clearer sentence such as "Removes a user from a
trip. This action may be performed by an admin or by the user themself
(self-removal)." so API consumers understand permitted callers.
Comment on lines
+1742
to
+1753
| rsvpUser := func(t *testing.T, userID string) { | ||
| t.Helper() | ||
| testkit.New(t). | ||
| Request(testkit.Request{ | ||
| App: app, | ||
| Route: fmt.Sprintf("/api/v1/trips/%s/activities/%s/rsvps", trip, activityID), | ||
| Method: testkit.POST, | ||
| UserID: &userID, | ||
| Body: models.ActivityRSVPRequestPayload{Status: models.RSVPStatusGoing}, | ||
| }). | ||
| AssertStatus(http.StatusOK) | ||
| } |
Contributor
There was a problem hiding this comment.
RSVP helper uses POST but the endpoint expects PUT.
Every other RSVP creation in this file (e.g., lines 1163, 1178, 1193, 1222, 1297, 1308, 1642) issues PUT to /api/v1/trips/{tripID}/activities/{activityID}/rsvps. This helper instead uses testkit.POST, which does not match the registered route. The helper’s AssertStatus(http.StatusOK) will fail, causing four of the six subtests (admin can remove…, proposer can remove…, regular member cannot remove…, non-member cannot remove…) to fail before they ever exercise the DELETE endpoint under test.
Proposed fix
- Request(testkit.Request{
- App: app,
- Route: fmt.Sprintf("/api/v1/trips/%s/activities/%s/rsvps", trip, activityID),
- Method: testkit.POST,
- UserID: &userID,
- Body: models.ActivityRSVPRequestPayload{Status: models.RSVPStatusGoing},
- }).
+ Request(testkit.Request{
+ App: app,
+ Route: fmt.Sprintf("/api/v1/trips/%s/activities/%s/rsvps", trip, activityID),
+ Method: testkit.PUT,
+ UserID: &userID,
+ Body: models.ActivityRSVPRequestPayload{Status: models.RSVPStatusGoing},
+ }).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| rsvpUser := func(t *testing.T, userID string) { | |
| t.Helper() | |
| testkit.New(t). | |
| Request(testkit.Request{ | |
| App: app, | |
| Route: fmt.Sprintf("/api/v1/trips/%s/activities/%s/rsvps", trip, activityID), | |
| Method: testkit.POST, | |
| UserID: &userID, | |
| Body: models.ActivityRSVPRequestPayload{Status: models.RSVPStatusGoing}, | |
| }). | |
| AssertStatus(http.StatusOK) | |
| } | |
| rsvpUser := func(t *testing.T, userID string) { | |
| t.Helper() | |
| testkit.New(t). | |
| Request(testkit.Request{ | |
| App: app, | |
| Route: fmt.Sprintf("/api/v1/trips/%s/activities/%s/rsvps", trip, activityID), | |
| Method: testkit.PUT, | |
| UserID: &userID, | |
| Body: models.ActivityRSVPRequestPayload{Status: models.RSVPStatusGoing}, | |
| }). | |
| AssertStatus(http.StatusOK) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/internal/tests/activity_test.go` around lines 1742 - 1753, The
rsvpUser helper is using testkit.POST but the RSVP route expects PUT; update the
helper function (rsvpUser) to use testkit.PUT instead of testkit.POST when
calling Request for the "/api/v1/trips/%s/activities/%s/rsvps" route so the
request matches the registered route and the AssertStatus(http.StatusOK) checks
succeed; keep the same Request body (models.ActivityRSVPRequestPayload{Status:
models.RSVPStatusGoing}) and other parameters unchanged.
Comment on lines
+1805
to
+1816
| t.Run("non-member cannot remove an RSVP", func(t *testing.T) { | ||
| rsvpUser(t, member) | ||
|
|
||
| testkit.New(t). | ||
| Request(testkit.Request{ | ||
| App: app, | ||
| Route: fmt.Sprintf("/api/v1/trips/%s/activities/%s/rsvps/%s", trip, activityID, member), | ||
| Method: testkit.DELETE, | ||
| UserID: &nonMember, | ||
| }). | ||
| AssertStatus(http.StatusNotFound) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Subtest name does not match the asserted behavior.
The subtest is titled "non-member cannot remove an RSVP" and expects 404 Not Found, but the scenario being verified (a non-member hitting the endpoint) is really an authorization/visibility check rather than a missing-RSVP check. Given the PR states non-admins should get 403 and non-members typically get 404 for trip-scoped routes, the current assertion is plausible, but it collides semantically with the preceding subtest (removing an RSVP that does not exist returns 404). Consider renaming to something like "non-member receives 404 (trip not visible)" to make the intent explicit and to avoid false positives if the service later starts returning 403 for this case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/internal/tests/activity_test.go` around lines 1805 - 1816, Rename the
subtest title for clarity: change the t.Run invocation currently named
"non-member cannot remove an RSVP" to something explicit like "non-member
receives 404 (trip not visible)" so the test intent (authorization/visibility
causing 404) is unambiguous and cannot be confused with the separate "removing
an RSVP that does not exist returns 404" case; update the t.Run call surrounding
the Request that targets fmt.Sprintf("/api/v1/trips/%s/activities/%s/rsvps/%s",
trip, activityID, member) with UserID set to &nonMember and
AssertStatus(http.StatusNotFound) to use the new descriptive name.
in-mai-space
previously approved these changes
Apr 17, 2026
afnantuffaha
force-pushed
the
admin-permissions
branch
from
April 17, 2026 20:15
c4c3075 to
e064437
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/internal/services/membership.go (1)
251-277: 🧹 Nitpick | 🔵 TrivialRedundant membership lookup.
IsMemberat line 253 andFindat line 262 hit the membership table for the same(userID, tripID)pair.Findalready returnserrs.ErrNotFoundwhen the row does not exist, so theIsMembercall can be dropped and the not-found case handled from theFinderror. This removes one DB round trip per delete.Proposed change
func (s *MembershipService) RemoveMember(ctx context.Context, tripID, userID uuid.UUID) error { - // Check if member exists - isMember, err := s.Membership.IsMember(ctx, tripID, userID) - if err != nil { - return err - } - if !isMember { - return errors.New("user is not a member of this trip") - } - - // Prevent removing the last admin membership, err := s.Membership.Find(ctx, userID, tripID) if err != nil { + if errors.Is(err, errs.ErrNotFound) { + return errors.New("user is not a member of this trip") + } return err } if membership.IsAdmin { admins, err := s.Membership.CountAdmins(ctx, tripID) if err != nil { return err } if admins <= 1 { return errs.BadRequest(errors.New("cannot remove the last admin of a trip")) } } return s.Membership.Delete(ctx, userID, tripID) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/services/membership.go` around lines 251 - 277, Remove the redundant Membership.IsMember DB call in RemoveMember: call s.Membership.Find(ctx, userID, tripID) directly and if it returns the repository not-found error (errs.ErrNotFound) return the existing "user is not a member of this trip" error, otherwise propagate other errors; keep the IsAdmin check, the admins count via s.Membership.CountAdmins(ctx, tripID), and final s.Membership.Delete(ctx, userID, tripID) unchanged. Ensure you reference the same parameter order used by Find and use errs.ErrNotFound (or the project’s not-found sentinel) to distinguish missing membership from other DB errors.
♻️ Duplicate comments (4)
backend/docs/swagger.json (1)
3185-3185:⚠️ Potential issue | 🟠 MajorMembership removal description is incorrect for self-removal behavior.
Line 3185 says only admins can remove members, but the endpoint also allows the target user to remove themself. This misstates the contract.
Proposed doc fix
- "description": "Removes a user from a trip. Only admins can remove members.", + "description": "Removes a user from a trip. Caller must be a trip admin or the target user themself.",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/docs/swagger.json` at line 3185, Update the operation description string that currently reads "Removes a user from a trip. Only admins can remove members." to accurately state that removal is allowed by admins or by the member themselves (self-removal). Locate the JSON object for the trip membership removal endpoint (the description field you changed) and replace the text with a concise contract: e.g., "Removes a user from a trip; can be performed by an admin or by the user themself (self-removal)." Ensure the description clearly mentions authorization rules for admins and self-removal and keep wording consistent with other endpoint docs.backend/docs/docs.go (1)
3191-3234:⚠️ Potential issue | 🟠 MajorFix authorization description mismatch for membership removal.
Line 3191 is incorrect. The endpoint allows trip admins to remove any member and allows users to remove themselves. The current text says only admins can remove members, which misdocuments behavior.
Suggested documentation update
- "description": "Removes a user from a trip. Only admins can remove members.", + "description": "Removes a user from a trip. Trip admins can remove any member, and users can remove themselves.",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/docs/docs.go` around lines 3191 - 3234, Update the operation description for the OpenAPI entry with operationId "removeMember" to reflect actual authorization rules: state that trip admins can remove any member and individual users may remove themselves; keep tags, parameters, responses unchanged and only modify the "description" (and optionally "summary") text to accurately document both admin-removal and self-removal behaviors so docs match the implementation.backend/internal/tests/activity_test.go (1)
1742-1753:⚠️ Potential issue | 🔴 CriticalRSVP setup helper uses the wrong HTTP method.
Line 1748 uses
POST, but this file’s RSVP endpoint usage isPUT(/rsvps). This can fail setup and prevent the delete-flow assertions from actually being tested.Proposed fix
rsvpUser := func(t *testing.T, userID string) { t.Helper() testkit.New(t). Request(testkit.Request{ App: app, Route: fmt.Sprintf("/api/v1/trips/%s/activities/%s/rsvps", trip, activityID), - Method: testkit.POST, + Method: testkit.PUT, UserID: &userID, Body: models.ActivityRSVPRequestPayload{Status: models.RSVPStatusGoing}, }). AssertStatus(http.StatusOK) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/tests/activity_test.go` around lines 1742 - 1753, The helper rsvpUser uses the wrong HTTP method constant (testkit.POST) when calling testkit.Request for the /api/v1/trips/{trip}/activities/{activityID}/rsvps endpoint; change the request Method to testkit.PUT in the rsvpUser helper so the RSVP setup actually hits the expected PUT handler (update the Method field inside the testkit.Request call in the rsvpUser function).backend/docs/swagger.yaml (1)
4027-4027:⚠️ Potential issue | 🟠 MajorMembership delete description is incorrect for self-removal behavior.
Line 4027 says only admins can remove members, but this endpoint also allows a user to remove themself. This mismatches the API contract and can mislead clients.
Proposed fix
- description: Removes a user from a trip. Only admins can remove members. + description: Removes a user from a trip. A trip admin can remove any member, + and a user can remove themself. The last remaining admin cannot be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/docs/swagger.yaml` at line 4027, Update the OpenAPI description for the "Membership delete" operation so it accurately reflects behavior: change the description string (the description field currently saying "Removes a user from a trip. Only admins can remove members.") to indicate that admins can remove other members and that a user may remove themselves; ensure you edit the description field for the Membership delete operation in backend/docs/swagger.yaml to mention both admin removal and self-removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/internal/services/membership.go`:
- Around line 261-276: The last-admin check in RemoveMember uses
s.Membership.Find, s.Membership.CountAdmins and then s.Membership.Delete outside
a transaction, allowing a race that can remove the final admin; wrap the
admin-count validation and the deletion in a single DB transaction or perform an
atomic conditional delete (e.g., delete only when admin count > 1) so the check
and delete cannot interleave; apply the same fix to DemoteFromAdmin (the code
that calls s.Membership.CountAdmins and updates membership to remove admin
rights) to ensure both operations are performed atomically or use SELECT ... FOR
UPDATE / a conditional SQL statement that fails when it would drop admins below
1.
In `@frontend/api/activities/usePostApiV1TripsTripidActivitiesActivityidRsvp.ts`:
- Line 27: The POST RSVP client path is singular but the backend router mounts
activityRSVPGroup.Post("", ...) under /rsvps (plural); update the backend
controller Swagger annotation that currently says `@Router` .../rsvp [post] to
`@Router` .../rsvps [post], then regenerate the OpenAPI/Swagger spec and re-run
the frontend client generator so frontend file
frontend/api/activities/usePostApiV1TripsTripidActivitiesActivityidRsvp.ts (and
related generated clients) use /rsvps to match the actual routes and keep
GET/DELETE endpoints consistent.
In `@frontend/api/memberships/useRemoveMember.ts`:
- Line 32: Update the OpenAPI operation (the DELETE route that generated
useRemoveMember / the operationId for removing a trip member) to reflect that
callers can be either an admin or the member themselves (self-leave) by changing
the summary/description text from “Only admins can remove members” to something
like “Admins can remove any member; members may remove themselves (self-leave).”
After updating that route's summary/description in the OpenAPI spec, regenerate
the Kubb client so frontend/api/memberships/useRemoveMember.ts and its JSDoc are
updated accordingly.
In `@frontend/app/`(app)/trips/[id]/activities/[id]/index.tsx:
- Around line 235-257: Replace the hard-coded inline style
backgroundColor:"#FFFFFF" used in the two early-return SafeAreaView blocks (the
isLoading and !activity branches) with the design-system token by removing the
inline backgroundColor and using the Box component's background/surface prop (or
the equivalent theme token) to set the background; update the SafeAreaView
wrapping to rely on Box's background (or pass the design-system token into
SafeAreaView if required by your UI library) so both loading and "Activity not
found" returns use the centralized theme color instead of "#FFFFFF".
- Around line 68-69: The membership query is running with empty ids causing a
false-negative isAdmin; update the useGetMembership call (the hook named
useGetMembership where you pass tripID and userId) to only run when both tripID
and userId are present by passing a query config with enabled: !!(tripID &&
userId) (mirror the pattern used in useGetActivity), so the request is skipped
until valid ids exist and UI permission checks rely on real data.
- Around line 178-199: The callback handleEditLocation currently assumes
res.data.geometry.location.lat/lng exist and sets locationSelectStore without a
cancellation path; update the locationSelectStore.set handler used in
handleEditLocation to (1) defensively read coordinates (e.g., const lat =
res?.data?.geometry?.location?.lat; const lng =
res?.data?.geometry?.location?.lng) and if either is undefined fall back to the
prediction-only branch (setLocationName(prediction.description ?? null) and skip
saveField), and (2) provide an onCancel (or pass a cancel callback into
locationSelectStore.set) that clears the pending handler (e.g., call
locationSelectStore.clear()) so the closure does not hold a stale saveField
reference when the user navigates away; reference functions: handleEditLocation,
locationSelectStore.set, getPlaceDetailsCustom, and saveField.
In `@frontend/app/`(app)/trips/[id]/housing/[id]/index.tsx:
- Around line 149-170: handleEditLocation currently calls
locationSelectStore.set(...) with only the select callback and then navigates;
follow the defensive pattern from settings by passing a cancel callback as the
second argument to locationSelectStore.set (e.g., a no-op or one that resets
state) and add a useEffect in this component that returns a cleanup calling
locationSelectStore.clear() so any stale callback is removed on unmount; update
references to handleEditLocation, locationSelectStore.set,
locationSelectStore.clear, useEffect, router.push, tripID and saveField
accordingly.
---
Outside diff comments:
In `@backend/internal/services/membership.go`:
- Around line 251-277: Remove the redundant Membership.IsMember DB call in
RemoveMember: call s.Membership.Find(ctx, userID, tripID) directly and if it
returns the repository not-found error (errs.ErrNotFound) return the existing
"user is not a member of this trip" error, otherwise propagate other errors;
keep the IsAdmin check, the admins count via s.Membership.CountAdmins(ctx,
tripID), and final s.Membership.Delete(ctx, userID, tripID) unchanged. Ensure
you reference the same parameter order used by Find and use errs.ErrNotFound (or
the project’s not-found sentinel) to distinguish missing membership from other
DB errors.
---
Duplicate comments:
In `@backend/docs/docs.go`:
- Around line 3191-3234: Update the operation description for the OpenAPI entry
with operationId "removeMember" to reflect actual authorization rules: state
that trip admins can remove any member and individual users may remove
themselves; keep tags, parameters, responses unchanged and only modify the
"description" (and optionally "summary") text to accurately document both
admin-removal and self-removal behaviors so docs match the implementation.
In `@backend/docs/swagger.json`:
- Line 3185: Update the operation description string that currently reads
"Removes a user from a trip. Only admins can remove members." to accurately
state that removal is allowed by admins or by the member themselves
(self-removal). Locate the JSON object for the trip membership removal endpoint
(the description field you changed) and replace the text with a concise
contract: e.g., "Removes a user from a trip; can be performed by an admin or by
the user themself (self-removal)." Ensure the description clearly mentions
authorization rules for admins and self-removal and keep wording consistent with
other endpoint docs.
In `@backend/docs/swagger.yaml`:
- Line 4027: Update the OpenAPI description for the "Membership delete"
operation so it accurately reflects behavior: change the description string (the
description field currently saying "Removes a user from a trip. Only admins can
remove members.") to indicate that admins can remove other members and that a
user may remove themselves; ensure you edit the description field for the
Membership delete operation in backend/docs/swagger.yaml to mention both admin
removal and self-removal.
In `@backend/internal/tests/activity_test.go`:
- Around line 1742-1753: The helper rsvpUser uses the wrong HTTP method constant
(testkit.POST) when calling testkit.Request for the
/api/v1/trips/{trip}/activities/{activityID}/rsvps endpoint; change the request
Method to testkit.PUT in the rsvpUser helper so the RSVP setup actually hits the
expected PUT handler (update the Method field inside the testkit.Request call in
the rsvpUser function).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 0120809c-3a9a-4f57-a105-1c77ed92fa3f
📒 Files selected for processing (25)
backend/docs/docs.gobackend/docs/swagger.jsonbackend/docs/swagger.yamlbackend/internal/controllers/activity.gobackend/internal/controllers/membership.gobackend/internal/repository/activity.gobackend/internal/repository/activity_rsvp.gobackend/internal/server/routers/activity.gobackend/internal/services/activity.gobackend/internal/services/membership.gobackend/internal/tests/activity_test.gobackend/internal/tests/membership_test.gofrontend/api/activities/index.tsfrontend/api/activities/useDeleteApiV1TripsTripidActivitiesActivityidRsvpsUserid.tsfrontend/api/activities/usePostApiV1TripsTripidActivitiesActivityidRsvp.tsfrontend/api/index.tsfrontend/api/memberships/useRemoveMember.tsfrontend/app/(app)/trips/[id]/activities/[id]/index.tsxfrontend/app/(app)/trips/[id]/components/entity-detail-screen.tsxfrontend/app/(app)/trips/[id]/components/link-pill.tsxfrontend/app/(app)/trips/[id]/components/members-going-section.tsxfrontend/app/(app)/trips/[id]/housing/[id]/index.tsxfrontend/index.tsfrontend/types/schema.gen.tsfrontend/types/types.gen.ts
Comment on lines
+261
to
276
| // Prevent removing the last admin | ||
| membership, err := s.Membership.Find(ctx, userID, tripID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if membership.IsAdmin { | ||
| admins, err := s.Membership.CountAdmins(ctx, tripID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if admins <= 1 { | ||
| return errs.BadRequest(errors.New("cannot remove the last admin of a trip")) | ||
| } | ||
| } | ||
|
|
||
| return s.Membership.Delete(ctx, userID, tripID) |
Contributor
There was a problem hiding this comment.
Race condition on last-admin check.
The CountAdmins query and the subsequent Delete are not performed in a transaction, so two concurrent RemoveMember calls targeting different admins can both observe admins == 2, both pass the guard, and both proceed to delete, leaving the trip with zero admins. Consider wrapping the count and delete in a single transaction (e.g., with SELECT ... FOR UPDATE on the trip's admin rows) or performing a conditional delete that fails when it would drop the admin count below 1. The same concern applies to DemoteFromAdmin at lines 312-327.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/internal/services/membership.go` around lines 261 - 276, The
last-admin check in RemoveMember uses s.Membership.Find,
s.Membership.CountAdmins and then s.Membership.Delete outside a transaction,
allowing a race that can remove the final admin; wrap the admin-count validation
and the deletion in a single DB transaction or perform an atomic conditional
delete (e.g., delete only when admin count > 1) so the check and delete cannot
interleave; apply the same fix to DemoteFromAdmin (the code that calls
s.Membership.CountAdmins and updates membership to remove admin rights) to
ensure both operations are performed atomically or use SELECT ... FOR UPDATE / a
conditional SQL statement that fails when it would drop admins below 1.
|
|
||
| /** | ||
| * @description Removes a user from a trip | ||
| * @description Removes a user from a trip. Only admins can remove members. |
Contributor
There was a problem hiding this comment.
JSDoc understates the allowed callers.
The description reads “Only admins can remove members,” but the backend also permits a user to remove themselves (self-leave). Since this is Kubb-generated, update the OpenAPI summary/description for this route so regeneration yields accurate client docs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/api/memberships/useRemoveMember.ts` at line 32, Update the OpenAPI
operation (the DELETE route that generated useRemoveMember / the operationId for
removing a trip member) to reflect that callers can be either an admin or the
member themselves (self-leave) by changing the summary/description text from
“Only admins can remove members” to something like “Admins can remove any
member; members may remove themselves (self-leave).” After updating that route's
summary/description in the OpenAPI spec, regenerate the Kubb client so
frontend/api/memberships/useRemoveMember.ts and its JSDoc are updated
accordingly.
Comment on lines
+68
to
+69
| // Fetch current user's membership to check admin status | ||
| const { data: myMembership } = useGetMembership(tripID ?? "", userId ?? ""); |
Contributor
There was a problem hiding this comment.
Guard the membership query on valid ids.
useGetMembership(tripID ?? "", userId ?? "") runs even when tripID or userId is missing, issuing a request against /trips//memberships/ and leaving isAdmin as false until the (failing) response settles. The neighboring useGetActivity already uses query: { enabled: !!(tripID && activityID) }; apply the same pattern here so permission-gated UI doesn't render under a false-negative admin flag on first paint.
Proposed fix
- // Fetch current user's membership to check admin status
- const { data: myMembership } = useGetMembership(tripID ?? "", userId ?? "");
+ // Fetch current user's membership to check admin status
+ const { data: myMembership } = useGetMembership(tripID ?? "", userId ?? "", {
+ query: { enabled: !!(tripID && userId) },
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/`(app)/trips/[id]/activities/[id]/index.tsx around lines 68 -
69, The membership query is running with empty ids causing a false-negative
isAdmin; update the useGetMembership call (the hook named useGetMembership where
you pass tripID and userId) to only run when both tripID and userId are present
by passing a query config with enabled: !!(tripID && userId) (mirror the pattern
used in useGetActivity), so the request is skipped until valid ids exist and UI
permission checks rely on real data.
Comment on lines
178
to
+199
| const handleEditLocation = useCallback(() => { | ||
| destinationSheetRef.current?.snapToIndex(0); | ||
| }, []); | ||
|
|
||
| const handleLocationSelected = useCallback( | ||
| async (location: SelectedLocation) => { | ||
| setLocationName(location.name); | ||
| setLocationLat(location.lat ?? null); | ||
| setLocationLng(location.lng ?? null); | ||
| await saveField({ | ||
| location_name: location.name, | ||
| location_lat: location.lat, | ||
| location_lng: location.lng, | ||
| }); | ||
| }, | ||
| [saveField], | ||
| ); | ||
| locationSelectStore.set(async (prediction) => { | ||
| try { | ||
| const res = await getPlaceDetailsCustom({ | ||
| place_id: prediction.place_id, | ||
| }); | ||
| const newName = | ||
| res.data.formatted_address || prediction.description || res.data.name; | ||
| setLocationName(newName); | ||
| setLocationLat(res.data.geometry.location.lat); | ||
| setLocationLng(res.data.geometry.location.lng); | ||
| await saveField({ | ||
| location_name: newName, | ||
| location_lat: res.data.geometry.location.lat, | ||
| location_lng: res.data.geometry.location.lng, | ||
| }); | ||
| } catch { | ||
| setLocationName(prediction.description ?? null); | ||
| } | ||
| }); | ||
| router.push(`/trips/${tripID}/search-location?mode=select`); | ||
| }, [tripID, saveField]); |
Contributor
There was a problem hiding this comment.
Handle optional geometry fields and the cancellation path.
Two concerns in this callback:
res.data.geometry.location.lat/.lngassumegeometryandgeometry.locationare populated, but permodelsPlaceDetailsResponseSchema/modelsPlaceGeometrySchemabothgeometryandlocation(and itslat/lng) are optional. If Google Places returns a result without geometry, this throws aTypeErrorinside the store callback and the user sees no feedback. Extract the coordinates defensively and fall back to the prediction-only branch when they are missing.locationSelectStore.setis called without a cancel callback, so the pending closure survives if the user backs out of the search-location screen and captures the (then stale)saveFieldreference for any later selection in a different flow. Pass anonCancel(or calllocationSelectStore.clear()on unmount) to release it.
Proposed fix
const handleEditLocation = useCallback(() => {
- locationSelectStore.set(async (prediction) => {
- try {
- const res = await getPlaceDetailsCustom({
- place_id: prediction.place_id,
- });
- const newName =
- res.data.formatted_address || prediction.description || res.data.name;
- setLocationName(newName);
- setLocationLat(res.data.geometry.location.lat);
- setLocationLng(res.data.geometry.location.lng);
- await saveField({
- location_name: newName,
- location_lat: res.data.geometry.location.lat,
- location_lng: res.data.geometry.location.lng,
- });
- } catch {
- setLocationName(prediction.description ?? null);
- }
- });
+ locationSelectStore.set(
+ async (prediction) => {
+ try {
+ const res = await getPlaceDetailsCustom({
+ place_id: prediction.place_id,
+ });
+ const loc = res.data.geometry?.location;
+ const newName =
+ res.data.formatted_address ||
+ prediction.description ||
+ res.data.name ||
+ null;
+ setLocationName(newName);
+ if (loc?.lat != null && loc?.lng != null) {
+ setLocationLat(loc.lat);
+ setLocationLng(loc.lng);
+ await saveField({
+ location_name: newName ?? undefined,
+ location_lat: loc.lat,
+ location_lng: loc.lng,
+ });
+ } else if (newName) {
+ await saveField({ location_name: newName });
+ }
+ } catch {
+ setLocationName(prediction.description ?? null);
+ }
+ },
+ () => locationSelectStore.clear(),
+ );
router.push(`/trips/${tripID}/search-location?mode=select`);
}, [tripID, saveField]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/`(app)/trips/[id]/activities/[id]/index.tsx around lines 178 -
199, The callback handleEditLocation currently assumes
res.data.geometry.location.lat/lng exist and sets locationSelectStore without a
cancellation path; update the locationSelectStore.set handler used in
handleEditLocation to (1) defensively read coordinates (e.g., const lat =
res?.data?.geometry?.location?.lat; const lng =
res?.data?.geometry?.location?.lng) and if either is undefined fall back to the
prediction-only branch (setLocationName(prediction.description ?? null) and skip
saveField), and (2) provide an onCancel (or pass a cancel callback into
locationSelectStore.set) that clears the pending handler (e.g., call
locationSelectStore.clear()) so the closure does not hold a stale saveField
reference when the user navigates away; reference functions: handleEditLocation,
locationSelectStore.set, getPlaceDetailsCustom, and saveField.
Comment on lines
+235
to
+257
| if (isLoading) { | ||
| return ( | ||
| <SafeAreaView style={{ flex: 1, backgroundColor: "#FFFFFF" }} edges={[]}> | ||
| <Box flex={1} justifyContent="center" alignItems="center"> | ||
| <Text variant="bodyDefault" color="gray500"> | ||
| Loading... | ||
| </Text> | ||
| </Box> | ||
| </SafeAreaView> | ||
| ); | ||
| } | ||
|
|
||
| if (!activity) { | ||
| return ( | ||
| <SafeAreaView style={{ flex: 1, backgroundColor: "#FFFFFF" }} edges={[]}> | ||
| <Box flex={1} justifyContent="center" alignItems="center"> | ||
| <Text variant="bodyDefault" color="gray500"> | ||
| Activity not found. | ||
| </Text> | ||
| </Box> | ||
| </SafeAreaView> | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Replace the hard-coded background color with a design-system token.
The loading and not-found early returns inline backgroundColor: "#FFFFFF". As per coding guidelines, "React/Expo UI must use a centralized design system; avoid hard-coded color values". Use the design-system Box background prop (or the equivalent surface token) so theme changes propagate here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/`(app)/trips/[id]/activities/[id]/index.tsx around lines 235 -
257, Replace the hard-coded inline style backgroundColor:"#FFFFFF" used in the
two early-return SafeAreaView blocks (the isLoading and !activity branches) with
the design-system token by removing the inline backgroundColor and using the Box
component's background/surface prop (or the equivalent theme token) to set the
background; update the SafeAreaView wrapping to rely on Box's background (or
pass the design-system token into SafeAreaView if required by your UI library)
so both loading and "Activity not found" returns use the centralized theme color
instead of "#FFFFFF".
Comment on lines
149
to
+170
| const handleEditLocation = useCallback(() => { | ||
| destinationSheetRef.current?.snapToIndex(0); | ||
| }, []); | ||
|
|
||
| const handleLocationSelected = useCallback( | ||
| async (location: SelectedLocation) => { | ||
| setLocationName(location.name); | ||
| setLocationLat(location.lat ?? null); | ||
| setLocationLng(location.lng ?? null); | ||
| await saveField({ | ||
| location_name: location.name, | ||
| location_lat: location.lat, | ||
| location_lng: location.lng, | ||
| }); | ||
| }, | ||
| [saveField], | ||
| ); | ||
| locationSelectStore.set(async (prediction) => { | ||
| try { | ||
| const res = await getPlaceDetailsCustom({ | ||
| place_id: prediction.place_id, | ||
| }); | ||
| const newName = | ||
| res.data.formatted_address || prediction.description || res.data.name; | ||
| setLocationName(newName); | ||
| setLocationLat(res.data.geometry.location.lat); | ||
| setLocationLng(res.data.geometry.location.lng); | ||
| await saveField({ | ||
| location_name: newName, | ||
| location_lat: res.data.geometry.location.lat, | ||
| location_lng: res.data.geometry.location.lng, | ||
| }); | ||
| } catch { | ||
| setLocationName(prediction.description ?? null); | ||
| } | ||
| }); | ||
| router.push(`/trips/${tripID}/search-location?mode=select`); | ||
| }, [tripID, saveField]); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect locationSelectStore implementation and all call sites to confirm reset semantics.
fd -t f 'locationSelectStore' -e ts -e tsx
rg -nP -C4 'locationSelectStore\.(set|clear|reset|get)\b' --type=ts --type=tsxRepository: GenerateNU/toggo
Length of output: 146
🏁 Script executed:
cat frontend/utilities/locationSelectStore.tsRepository: GenerateNU/toggo
Length of output: 836
🏁 Script executed:
fd -type f -name "*search-location*" -o -path "*search*location*"Repository: GenerateNU/toggo
Length of output: 230
🏁 Script executed:
rg "locationSelectStore" --type ts --type js -n -B2 -A2Repository: GenerateNU/toggo
Length of output: 9894
🏁 Script executed:
wc -l frontend/app/\(app\)/trips/\[id\]/housing/\[id\]/index.tsxRepository: GenerateNU/toggo
Length of output: 116
🏁 Script executed:
rg "useEffect|locationSelectStore" frontend/app/\(app\)/trips/\[id\]/housing/\[id\]/index.tsx -nRepository: GenerateNU/toggo
Length of output: 282
🏁 Script executed:
sed -n '95,110p' frontend/app/\(app\)/trips/\[id\]/housing/\[id\]/index.tsxRepository: GenerateNU/toggo
Length of output: 659
🏁 Script executed:
sed -n '140,175p' frontend/app/\(app\)/trips/\[id\]/housing/\[id\]/index.tsxRepository: GenerateNU/toggo
Length of output: 1165
Follow the defensive pattern used in settings for locationSelectStore cleanup.
The housing file currently passes only a select callback to locationSelectStore.set() without a cancel handler or useEffect cleanup. Although the search-location route's cleanup will eventually clear the callback on unmount, adopt the safer pattern used in settings/index.tsx: pass a cancel callback to locationSelectStore.set() (as the second argument) and add a useEffect cleanup that calls locationSelectStore.clear(). This prevents stale callbacks from persisting if navigation is interrupted or races occur.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/`(app)/trips/[id]/housing/[id]/index.tsx around lines 149 - 170,
handleEditLocation currently calls locationSelectStore.set(...) with only the
select callback and then navigates; follow the defensive pattern from settings
by passing a cancel callback as the second argument to locationSelectStore.set
(e.g., a no-op or one that resets state) and add a useEffect in this component
that returns a cleanup calling locationSelectStore.clear() so any stale callback
is removed on unmount; update references to handleEditLocation,
locationSelectStore.set, locationSelectStore.clear, useEffect, router.push,
tripID and saveField accordingly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/app/`(app)/trips/[id]/settings/members.tsx:
- Line 1: Prettier formatting failed for this file (the import line referencing
useMembersList is not formatted to project standards); run the formatter (e.g.,
bun format:check and if it fails run bun format or prettier --write), apply the
changes, then stage and commit the updated file so the CI Prettier check passes
before merging.
- Around line 289-317: handleRemove and handlePromote currently swallow all
errors and always show a generic retry message; instead catch the
ResponseErrorConfig coming from useRemoveMember/usePromoteToAdmin (typed as
ResponseErrorConfig<RemoveMember400 | 401 | 403 | 404 | 500>) and map its status
to user-friendly text: 400 -> "Cannot remove/promote: this is the last admin.",
403 -> "You don’t have permission to remove/promote that member.", 404 -> "That
membership no longer exists.", default/other -> generic "Couldn't remove/promote
member. Please try again."; update the async onPress handlers in handleRemove
and handlePromote to inspect the caught error (err.status or
err.response?.status depending on the error shape), pick the proper message, and
pass it into toast.show instead of the current generic message.
- Around line 151-241: Extract the duplicated optimistic-update scaffolding into
a reusable helper (e.g., buildMembersOptimisticOptions(queryClient, transform)
or a hook useOptimisticMembersMutation(transform)) that returns { onMutate,
onError, onSettled } and use it for both promoteToAdminMutation and
removeMemberMutation so only the per-item transform (map to set is_admin vs
filter out user) is passed in; inside the helper use
queryClient.cancelQueries/getQueryData/setQueryData/invalidateQueries against
the ["members", tripID] key and return/restore the previousMembers, and replace
loose oldData: any with the proper InfiniteData<GetTripMembersQueryResponse>
type (the same type used by useMembersList) to keep the cache shape
type-checked.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: a5f31c44-64e8-4168-b355-38a5acad6f31
📒 Files selected for processing (2)
backend/internal/services/activity.gofrontend/app/(app)/trips/[id]/settings/members.tsx
Comment on lines
+151
to
+241
| const promoteToAdminMutation = usePromoteToAdmin({ | ||
| mutation: { | ||
| onMutate: async ({ tripID, userID }) => { | ||
| await queryClient.cancelQueries({ queryKey: ["members", tripID] }); | ||
|
|
||
| const previousMembers = queryClient.getQueryData([ | ||
| "members", | ||
| tripID, | ||
| ]); | ||
|
|
||
| queryClient.setQueryData(["members", tripID], (oldData: any) => { | ||
| if (!oldData?.pages) return oldData; | ||
|
|
||
| return { | ||
| ...oldData, | ||
| pages: oldData.pages.map((page: any) => ({ | ||
| ...page, | ||
| items: page.items?.map( | ||
| (item: ModelsMembershipAPIResponse) => | ||
| item.user_id === userID | ||
| ? { ...item, is_admin: true } | ||
| : item | ||
| ), | ||
| })), | ||
| }; | ||
| }); | ||
|
|
||
| return { previousMembers }; | ||
| }, | ||
|
|
||
| onError: (_err, variables, context) => { | ||
| if (context?.previousMembers) { | ||
| queryClient.setQueryData( | ||
| ["members", variables.tripID], | ||
| context.previousMembers | ||
| ); | ||
| } | ||
| }, | ||
|
|
||
| onSettled: (_data, _error, variables) => { | ||
| queryClient.invalidateQueries({ | ||
| queryKey: ["members", variables.tripID], | ||
| }); | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| const removeMemberMutation = useRemoveMember({ | ||
| mutation: { | ||
| onMutate: async ({ tripID, userID }) => { | ||
| await queryClient.cancelQueries({ queryKey: ["members", tripID] }); | ||
|
|
||
| const previousMembers = queryClient.getQueryData([ | ||
| "members", | ||
| tripID, | ||
| ]); | ||
|
|
||
| queryClient.setQueryData(["members", tripID], (oldData: any) => { | ||
| if (!oldData?.pages) return oldData; | ||
|
|
||
| return { | ||
| ...oldData, | ||
| pages: oldData.pages.map((page: any) => ({ | ||
| ...page, | ||
| items: page.items?.filter( | ||
| (item: ModelsMembershipAPIResponse) => | ||
| item.user_id !== userID | ||
| ), | ||
| })), | ||
| }; | ||
| }); | ||
|
|
||
| return { previousMembers }; | ||
| }, | ||
|
|
||
| onError: (_err, variables, context) => { | ||
| if (context?.previousMembers) { | ||
| queryClient.setQueryData( | ||
| ["members", variables.tripID], | ||
| context.previousMembers | ||
| ); | ||
| } | ||
| }, | ||
|
|
||
| onSettled: (_data, _error, variables) => { | ||
| queryClient.invalidateQueries({ | ||
| queryKey: ["members", variables.tripID], | ||
| }); | ||
| }, | ||
| }, | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract the duplicated optimistic-update scaffolding.
promoteToAdminMutation and removeMemberMutation share the same onMutate / onError / onSettled structure against ["members", tripID]: cancel queries, snapshot previous cache, transform pages[].items, roll back on error, invalidate on settle. Only the per-item transform differs (map vs filter). This duplication will grow when more admin actions are added and makes it easy for the two paths to drift (e.g. one gets a bug fix and the other doesn't).
Extract a helper such as useOptimisticMembersMutation(transform: (items, userID) => items) in the membership API layer, or a generic buildMembersOptimisticOptions(queryClient, transform) that returns the { onMutate, onError, onSettled } object. The two mutations then reduce to a one-line transform each.
While refactoring, consider replacing oldData: any with the InfiniteData<GetTripMembersQueryResponse> type used by useMembersList so the cache shape is type-checked.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/`(app)/trips/[id]/settings/members.tsx around lines 151 - 241,
Extract the duplicated optimistic-update scaffolding into a reusable helper
(e.g., buildMembersOptimisticOptions(queryClient, transform) or a hook
useOptimisticMembersMutation(transform)) that returns { onMutate, onError,
onSettled } and use it for both promoteToAdminMutation and removeMemberMutation
so only the per-item transform (map to set is_admin vs filter out user) is
passed in; inside the helper use
queryClient.cancelQueries/getQueryData/setQueryData/invalidateQueries against
the ["members", tripID] key and return/restore the previousMembers, and replace
loose oldData: any with the proper InfiniteData<GetTripMembersQueryResponse>
type (the same type used by useMembersList) to keep the cache shape
type-checked.
Comment on lines
+289
to
+317
| const handleRemove = (member: ModelsMembershipAPIResponse) => { | ||
| Alert.alert( | ||
| "Remove Member?", | ||
| `${member.name ?? "This member"} will be removed.`, | ||
| [ | ||
| { text: "Cancel", style: "cancel" }, | ||
| { | ||
| text: "Remove", | ||
| style: "destructive", | ||
| onPress: async () => { | ||
| try { | ||
| await removeMemberMutation.mutateAsync({ | ||
| tripID, | ||
| userID: member.user_id!, | ||
| }); | ||
|
|
||
| toast.show({ | ||
| message: `${member.name ?? "Member"} has been removed.`, | ||
| }); | ||
| } catch { | ||
| toast.show({ | ||
| message: "Couldn't remove member. Please try again.", | ||
| }); | ||
| } | ||
| }, | ||
| }, | ||
| ] | ||
| ); | ||
| }; |
Contributor
There was a problem hiding this comment.
Surface backend-specific failure reasons to the user.
The backend for DELETE /trips/:tripID/memberships/:userID distinguishes meaningful states: 400 when removing the last admin, 403 when a non-admin targets another member, and 404 when the membership is missing. The current catch {} discards that signal and always renders "Couldn't remove member. Please try again.", so an admin who tries to remove the last remaining admin gets an incorrect prompt to retry an operation that will never succeed. handlePromote (lines 260-287) has the same problem.
Read the error from useRemoveMember/usePromoteToAdmin (typed as ResponseErrorConfig<RemoveMember400 | 401 | 403 | 404 | 500>) and map the status to a user-friendly message, e.g. a dedicated copy for the last-admin case.
Sketch
- } catch {
- toast.show({
- message: "Couldn't remove member. Please try again.",
- });
- }
+ } catch (err) {
+ const status = (err as { response?: { status?: number } })?.response?.status;
+ const message =
+ status === 400
+ ? "You can't remove the last admin of this trip."
+ : status === 403
+ ? "Only admins can remove members."
+ : status === 404
+ ? "That member is no longer in this trip."
+ : "Couldn't remove member. Please try again.";
+ toast.show({ message });
+ }As per coding guidelines: "errors shown to users must be clear and user-friendly".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleRemove = (member: ModelsMembershipAPIResponse) => { | |
| Alert.alert( | |
| "Remove Member?", | |
| `${member.name ?? "This member"} will be removed.`, | |
| [ | |
| { text: "Cancel", style: "cancel" }, | |
| { | |
| text: "Remove", | |
| style: "destructive", | |
| onPress: async () => { | |
| try { | |
| await removeMemberMutation.mutateAsync({ | |
| tripID, | |
| userID: member.user_id!, | |
| }); | |
| toast.show({ | |
| message: `${member.name ?? "Member"} has been removed.`, | |
| }); | |
| } catch { | |
| toast.show({ | |
| message: "Couldn't remove member. Please try again.", | |
| }); | |
| } | |
| }, | |
| }, | |
| ] | |
| ); | |
| }; | |
| const handleRemove = (member: ModelsMembershipAPIResponse) => { | |
| Alert.alert( | |
| "Remove Member?", | |
| `${member.name ?? "This member"} will be removed.`, | |
| [ | |
| { text: "Cancel", style: "cancel" }, | |
| { | |
| text: "Remove", | |
| style: "destructive", | |
| onPress: async () => { | |
| try { | |
| await removeMemberMutation.mutateAsync({ | |
| tripID, | |
| userID: member.user_id!, | |
| }); | |
| toast.show({ | |
| message: `${member.name ?? "Member"} has been removed.`, | |
| }); | |
| } catch (err) { | |
| const status = (err as { response?: { status?: number } })?.response?.status; | |
| const message = | |
| status === 400 | |
| ? "You can't remove the last admin of this trip." | |
| : status === 403 | |
| ? "Only admins can remove members." | |
| : status === 404 | |
| ? "That member is no longer in this trip." | |
| : "Couldn't remove member. Please try again."; | |
| toast.show({ message }); | |
| } | |
| }, | |
| }, | |
| ] | |
| ); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/`(app)/trips/[id]/settings/members.tsx around lines 289 - 317,
handleRemove and handlePromote currently swallow all errors and always show a
generic retry message; instead catch the ResponseErrorConfig coming from
useRemoveMember/usePromoteToAdmin (typed as ResponseErrorConfig<RemoveMember400
| 401 | 403 | 404 | 500>) and map its status to user-friendly text: 400 ->
"Cannot remove/promote: this is the last admin.", 403 -> "You don’t have
permission to remove/promote that member.", 404 -> "That membership no longer
exists.", default/other -> generic "Couldn't remove/promote member. Please try
again."; update the async onPress handlers in handleRemove and handlePromote to
inspect the caught error (err.status or err.response?.status depending on the
error shape), pick the proper message, and pass it into toast.show instead of
the current generic message.
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
3 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.
Description
Enforces authorization rules for member removal and activity RSVP removal across the trip and activity layers.
Remove member from trip
DELETE /trips/:tripID/memberships/:userIDnow requires the caller to be an admin or the user themselves. Non-admins attempting to remove another member receive a 403.Activity edit/delete
activity-crudPR. Proposer or admin only; regular members receive 403. No changes in this PR.Remove member from activity RSVP
DELETE /trips/:tripID/activities/:activityID/rsvps/:userIDactivity_rsvpstable schema supports targeted deletion via its composite PK(trip_id, activity_id, user_id).How has this been tested?
22 integration tests across
membership_test.goandactivity_test.gocovering:Membership removal
Activity RSVP removal
Checklist
User-Visible Changes
Changes by Category
Features
DELETE /api/v1/trips/{tripID}/activities/{activityID}/rsvps/{userID}for removing activity RSVPs.canEditandcanManageMembersflags).Fixes
sql.ErrNoRowsmapping).Improvements
useDeleteApiV1TripsTripidActivitiesActivityidRsvpsUserid).Infrastructure
membership_test.gocovering admin constraints and non-admin rejection; 12+ inactivity_test.gocovering RSVP deletion authorization and error cases.Author Contribution Summary