Repository navigation
fix(api): send the active session id when leaving an organization - #1003
Conversation
OrganizationMembership.delete() called UserApi.deleteMembership without a
session id, so DELETE /v1/me/organization_memberships/{organization_id}
went out without _clerk_session_id. Every other caller of the /me
organization endpoints passes currentSessionId(); this one now does too.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: clerk/clerk-android/.coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. 2 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour. 📝 WalkthroughWalkthrough
ChangesOrganization membership deletion
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to Membership deletion now includes session context, and the test verifies the request path and session parameter. No actionable risk preventing merge is established. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Review
Two independent reviews (Grok 4.7 Extra High Fast and Muse Spark 1.3 Extra High) looked at sending the active session id from OrganizationMembership.delete().
Consensus: The production change is correct. UserApi.deleteMembership defaults _clerk_session_id to null, and UrlAppendingMiddleware only adds _is_native. Passing sessionId = currentSessionId() matches the other /me organization callers and is what stops the multi-session 401. Nothing previously filed on this PR was reopened. CodeRabbit’s docstring-coverage warning and the author’s note that this has not been emulator-tested were left as-is.
Risk: Low. This is one call site using the same session helper as the rest of the organization API. When there is no session, the query param stays absent, which is today’s behavior.
Merge: Yes. This can merge as is.
Approval: I would approve.
Findings
Consider (one reviewer, non-blocking): OrganizationMembershipDeleteTest checks the path and _clerk_session_id, and it does not check the HTTP method or that delete() succeeded. The stub body is {}. This endpoint’s success body is decoded as ClientPiggybackedResponse, which requires response, so {} does not decode and delete() returns a failure while the test still passes. The session-id assertion still holds, because the request is recorded before decoding and requests.single() fails if no request is sent. See the inline comment. I would not hold the PR for it.
Dismissed: Missing KDoc (already flagged by CodeRabbit). Emulator coverage (already called out by the author; this unit test covers the query param the bug is about).
No critical or warning issues should block merge.
Sent by Cursor Automation: Multi Model Code Review
The stub body was `{}`, which does not decode as a ClientPiggybackedResponse,
so delete() returned a Failure and the test only proved a request was sent.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
swolfand
left a comment
There was a problem hiding this comment.
One-line fix that matches the other /me organization callers, and the test fails without it. LGTM.


What & why
OrganizationMembership.delete()calledClerkApi.user.deleteMembership(organization.id)without a session id.UserApi.deleteMembershipdefaults_clerk_session_idto null, andUrlAppendingMiddlewareonly adds_is_native. As a result,DELETE /v1/me/organization_memberships/{organization_id}went out with no session id.SessionApi.revokeSession. There, emulator testing with two signed-in sessions showed the/meendpoint returning 401authentication_invalidwithout_clerk_session_id, and 200 with it./meorganization endpoints passessessionId = currentSessionId().delete()now does the same.OrganizationMembershipDeleteTestfollowsSessionRevokeTestfrom fix(api): fix deletePassword and revokeSession, which threw on first call #997. It runsdelete()against a real RetrofitUserApiwith two sessions on the client, and checks that the request URL carries the active session id. Before the fix it failed withexpected:<sess_active> but was:<null>.What to focus on
UserApi.kt, so it does not conflict with fix(api): fix deletePassword and revokeSession, which threw on first call #997. The two can merge in either order.🤖 Generated with Claude Code
Summary by CodeRabbit