fix: publish ownership events after commit - #1478
Merged
Merged
Conversation
📝 WalkthroughWalkthrough소유권 이전에 원자적 스토어 계약과 입력·결과 타입을 추가했습니다. 인메모리 및 Drizzle 구현체가 역할 변경을 처리하며, Manager와 Service는 커밋 후 멤버십 이벤트를 발행합니다. 실패, 동시성, 재시도, 이벤트 오류 테스트와 API 문서가 갱신되었습니다. Changes멤버십 소유권 이전
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MembershipManager
participant MembershipStore
participant TransactionManager
participant EventPublisher
MembershipManager->>MembershipStore: transferOwnership(input)
MembershipStore->>TransactionManager: 역할 변경 트랜잭션 실행
TransactionManager-->>MembershipStore: OwnershipTransferResult
MembershipStore-->>MembershipManager: previousToRole 반환
MembershipManager->>EventPublisher: publishAfterCommit(MembershipUpdatedEvent)
EventPublisher-->>MembershipManager: 커밋 후 이벤트 발행 또는 즉시 발행 폴백
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. Comment |
📊 Benchmark Results✅ All benchmarks passed
Updated: 2026-07-25T09:04:40.139Z · Commit: ea111df |
kang-heewon
force-pushed
the
fix/1464-atomic-ownership-transfer
branch
from
July 25, 2026 07:05
9cf8fa0 to
e99910b
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/membership-core/src/libs/MembershipManager.ts (2)
115-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
transferOwnership와publishAfterCommitOrNow가 두 클래스에 완전히 동일하게 중복 구현되어 있습니다. 원자적 이전 위임(store.transferOwnership)과 커밋-후 이벤트 발행(publishAfterCommit→ 실패 시publishSafely폴백) 로직 자체는 정확하지만, 이 핵심 정합성 로직이 두 파일에 각각 존재해 한쪽만 수정될 경우 두 구현체 간 실패 시맨틱이 어긋날 위험이 있습니다.
packages/membership-core/src/libs/MembershipManager.ts#L115-L193:transferOwnership과publishAfterCommitOrNow를 공유 헬퍼(함수 또는 공통 베이스 클래스)로 추출합니다.packages/membership-core/src/libs/MembershipService.ts#L129-L207: 동일한 공유 헬퍼를 사용하도록 변경합니다.🤖 Prompt for 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. In `@packages/membership-core/src/libs/MembershipManager.ts` around lines 115 - 193, Extract the duplicated ownership-transfer and commit-aware event-publishing logic from MembershipManager.ts lines 115-193 and MembershipService.ts lines 129-207 into a shared helper or common base abstraction. Update both MembershipManager.transferOwnership and MembershipService.transferOwnership to use it, preserving the atomic store.transferOwnership call and publishAfterCommit fallback to publishSafely semantics in both files.
115-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
transferOwnership/publishAfterCommitOrNow로직이MembershipService.ts와 완전히 동일합니다.원자적 이전 위임 및 커밋-후 이벤트 발행 흐름 자체는 정확합니다(
previousToRole을 스토어에서 받아oldRole로 사용,EventAfterCommitRequiresActiveTransactionProblem만 폴백). 다만 이 신규 로직이MembershipService.ts에 바이트 단위로 중복되어 있어, 이후 한쪽만 수정되면 커밋 순서·원자성 보장이 두 구현체 간에 어긋날 위험이 있습니다. 공유 헬퍼(예: 별도 함수 또는 공통 베이스 클래스)로 추출하는 것을 권장합니다.export async function transferOwnershipAtomic( store: MembershipStore, eventPublisher: EventPublisher, tenantId: string, fromUserId: string, toUserId: string, ): Promise<void> { const { previousToRole } = await store.transferOwnership({ tenantId, fromUserId, toUserId }); // ...publishAfterCommitOrNow 호출 두 번을 여기로 이동 }Also applies to: 182-193
🤖 Prompt for 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. In `@packages/membership-core/src/libs/MembershipManager.ts` around lines 115 - 139, Extract the duplicated transferOwnership flow from MembershipManager.transferOwnership and MembershipService.transferOwnership into a shared helper or common base abstraction. The shared implementation must retain store.transferOwnership, previousToRole handling, event ordering, and publishAfterCommitOrNow behavior, with both callers delegating to it so future changes cannot diverge.packages/membership-core/src/libs/MembershipService.ts (1)
129-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
MembershipManager.ts의transferOwnership/publishAfterCommitOrNow와 완전히 동일한 로직입니다.두 클래스가 핵심 원자성/이벤트 발행 로직을 각자 보유하고 있어 향후 변경 시 동기화가 누락될 위험이 있습니다.
MembershipManager.ts쪽 코멘트에서 제안한 공유 헬퍼 추출을 함께 적용하는 것을 권장합니다.Also applies to: 196-207
🤖 Prompt for 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. In `@packages/membership-core/src/libs/MembershipService.ts` around lines 129 - 153, Extract the shared ownership-transfer and event-publication logic used by MembershipService.transferOwnership and MembershipManager.transferOwnership/publishAfterCommitOrNow into a common helper. Update both classes to delegate to that helper while preserving the existing transaction atomicity and emitted MembershipUpdatedEvent payloads.
🤖 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 @.changeset/atomic-membership-ownership.md:
- Around line 2-3: Update the `@croco/membership-core` version bump in the
changeset from minor to major, reflecting the new abstract
MembershipStore.transferOwnership(...) requirement; leave the
`@croco/membership-drizzle` patch bump unchanged.
In `@packages/membership-core/src/libs/InMemoryMembershipStore.ts`:
- Around line 48-82: Update transferOwnership in InMemoryMembershipStore to
handle input.fromUserId === input.toUserId consistently with MembershipManager,
MembershipService, and the Drizzle store: either reject the transfer using the
established validation/problem or ensure both stores return the same
previousToRole and event payload. Apply the guard before mutating storage so
same-user transfers cannot produce divergent results.
In `@packages/membership-core/src/libs/MembershipManager.ts`:
- Around line 4-7: Update the EventPublisher imports in
packages/membership-core/src/libs/MembershipManager.ts lines 4-7 and
packages/membership-core/src/libs/MembershipService.ts lines 4-7 to use the type
modifier, while keeping EventAfterCommitRequiresActiveTransactionProblem as a
value import.
In `@packages/membership-core/src/libs/MembershipStore.ts`:
- Around line 18-24: Update MembershipStore.transferOwnership to document that
fromUserId and toUserId must differ, and validate this precondition before any
changes in DrizzleMembershipStore.transferOwnership, rejecting equal IDs with
the appropriate Problem subclass. Regenerate or update the API documentation in
packages/docs/src/content/docs/api/membership-core/src/classes/MembershipStore.md
(lines 182-199) and
packages/docs/src/content/docs/api/membership-core/src/classes/InMemoryMembershipStore.md
(lines 202-223) to include the same precondition.
In `@packages/membership-core/src/tests/MembershipManager.spec.ts`:
- Around line 402-430: Update the rollback test around
MembershipManager.transferOwnership and the fake TransactionContext to model an
explicit rollback path that discards registered after-commit hooks without
executing them. Invoke that rollback path after asserting hooks were deferred,
then verify publishCommittedEvent remains uncalled; do not merely clear
afterCommitHooks directly, and extend the context shape only as needed for this
test.
In `@packages/membership-core/src/tests/MembershipService.spec.ts`:
- Around line 49-51: MembershipService.transferOwnership lacks coverage for
atomicity, concurrency, retries, rollback, and post-commit event behavior. Add
corresponding tests in MembershipService.spec.ts, using the scenarios from
MembershipManager.spec.ts around its transfer tests, and exercise
transferOwnership plus publishAfterCommit/publishAfterCommitOrNow to verify
state preservation on store failure, retry behavior, a single winner for
concurrent transfers, event ordering after commit, and event disposal on
rollback.
In `@packages/membership-core/src/tests/MembershipStore.spec.ts`:
- Around line 142-161: Add store-level failure-path tests alongside “should
transfer ownership atomically” in MembershipStore.spec.ts: verify
transferOwnership rejects with OwnershipTransferRequiredProblem when the source
user is not an owner, and rejects with MembershipNotFoundProblem when the target
is missing while preserving the existing owner role. Include setup for each
scenario and assert the relevant error type and unchanged ownership state.
In `@packages/membership-drizzle/src/tests/DrizzleMembershipStore.spec.ts`:
- Around line 2-7: Separate the type-only imports Membership and
MembershipCreateInput from the value imports in DrizzleMembershipStore.spec.ts
by using a dedicated import type declaration, while keeping
MembershipNotFoundProblem and OwnershipTransferRequiredProblem in the regular
import block and preserving the existing import ordering.
---
Outside diff comments:
In `@packages/membership-core/src/libs/MembershipManager.ts`:
- Around line 115-193: Extract the duplicated ownership-transfer and
commit-aware event-publishing logic from MembershipManager.ts lines 115-193 and
MembershipService.ts lines 129-207 into a shared helper or common base
abstraction. Update both MembershipManager.transferOwnership and
MembershipService.transferOwnership to use it, preserving the atomic
store.transferOwnership call and publishAfterCommit fallback to publishSafely
semantics in both files.
- Around line 115-139: Extract the duplicated transferOwnership flow from
MembershipManager.transferOwnership and MembershipService.transferOwnership into
a shared helper or common base abstraction. The shared implementation must
retain store.transferOwnership, previousToRole handling, event ordering, and
publishAfterCommitOrNow behavior, with both callers delegating to it so future
changes cannot diverge.
In `@packages/membership-core/src/libs/MembershipService.ts`:
- Around line 129-153: Extract the shared ownership-transfer and
event-publication logic used by MembershipService.transferOwnership and
MembershipManager.transferOwnership/publishAfterCommitOrNow into a common
helper. Update both classes to delegate to that helper while preserving the
existing transaction atomicity and emitted MembershipUpdatedEvent payloads.
🪄 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 Plus
Run ID: 94b9221b-3f4f-47e4-90da-d0cb1f430eca
📒 Files selected for processing (17)
.changeset/atomic-membership-ownership.mdpackages/docs/src/content/docs/api/membership-core/src/classes/InMemoryMembershipStore.mdpackages/docs/src/content/docs/api/membership-core/src/classes/MembershipStore.mdpackages/docs/src/content/docs/api/membership-core/src/type-aliases/OwnershipTransferInput.mdpackages/docs/src/content/docs/api/membership-core/src/type-aliases/OwnershipTransferResult.mdpackages/docs/src/content/docs/api/membership-drizzle/src/classes/DrizzleMembershipStore.mdpackages/membership-core/src/libs/InMemoryMembershipStore.tspackages/membership-core/src/libs/MembershipManager.tspackages/membership-core/src/libs/MembershipService.tspackages/membership-core/src/libs/MembershipStore.tspackages/membership-core/src/libs/types.tspackages/membership-core/src/tests/MembershipManager.spec.tspackages/membership-core/src/tests/MembershipService.spec.tspackages/membership-core/src/tests/MembershipStore.spec.tspackages/membership-drizzle/src/libs/DrizzleMembershipStore.tspackages/membership-drizzle/src/tests/DrizzleMembershipStore.spec.tspublic-api-surface.snapshot.json
kang-heewon
force-pushed
the
fix/1464-atomic-ownership-transfer
branch
from
July 25, 2026 07:33
e99910b to
676f6e6
Compare
Member
Author
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
kang-heewon
force-pushed
the
fix/1464-atomic-ownership-transfer
branch
from
July 25, 2026 08:20
676f6e6 to
cf436e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@packages/docs/src/content/docs/api/membership-core/src/classes/SameUserOwnershipTransferProblem.md`:
- Around line 7-14: Update the JSDoc for the MembershipProblems.ts
SameUserOwnershipTransferProblem class to describe the
SAME_USER_OWNERSHIP_TRANSFER error and prohibit transferring ownership to the
same user, replacing the copied seat-limit wording. Regenerate
SameUserOwnershipTransferProblem.md so its summary and description reflect the
corrected source documentation.
In `@packages/membership-drizzle/src/tests/DrizzleMembershipStore.spec.ts`:
- Around line 62-64: Update the mockTxManager.run implementation in
DrizzleMembershipStore.spec.ts to emulate transaction isolation and rollback
instead of directly executing against shared state. Ensure failed operations
restore the pre-transaction state, and strengthen the failure tests around the
existing-owner demotion to assert that the owner remains "owner" after promotion
fails.
🪄 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 Plus
Run ID: 4deb11dc-485e-4ab5-b136-da027a0fdcb5
⛔ Files ignored due to path filters (1)
packages/problems-core/src/generated/problem-code-registry.tsis excluded by!**/generated/**
📒 Files selected for processing (23)
.changeset/atomic-membership-ownership.mddocs/problem-code-registry.jsonpackages/docs/src/content/docs/api/membership-core/src/classes/InMemoryMembershipStore.mdpackages/docs/src/content/docs/api/membership-core/src/classes/MembershipStore.mdpackages/docs/src/content/docs/api/membership-core/src/classes/SameUserOwnershipTransferProblem.mdpackages/docs/src/content/docs/api/membership-core/src/type-aliases/OwnershipTransferInput.mdpackages/docs/src/content/docs/api/membership-core/src/type-aliases/OwnershipTransferResult.mdpackages/docs/src/content/docs/api/membership-drizzle/src/classes/DrizzleMembershipStore.mdpackages/docs/src/content/docs/api/problems-core/src/classes/Problem.mdpackages/docs/src/content/docs/en/reference/problem-recovery-cookbook.mdpackages/membership-core/src/index.tspackages/membership-core/src/libs/InMemoryMembershipStore.tspackages/membership-core/src/libs/MembershipManager.tspackages/membership-core/src/libs/MembershipService.tspackages/membership-core/src/libs/MembershipStore.tspackages/membership-core/src/libs/problems/MembershipProblems.tspackages/membership-core/src/libs/types.tspackages/membership-core/src/tests/MembershipManager.spec.tspackages/membership-core/src/tests/MembershipService.spec.tspackages/membership-core/src/tests/MembershipStore.spec.tspackages/membership-drizzle/src/libs/DrizzleMembershipStore.tspackages/membership-drizzle/src/tests/DrizzleMembershipStore.spec.tspublic-api-surface.snapshot.json
kang-heewon
force-pushed
the
fix/1464-atomic-ownership-transfer
branch
from
July 25, 2026 08:32
cf436e1 to
98d0480
Compare
kang-heewon
force-pushed
the
fix/1464-atomic-ownership-transfer
branch
from
July 25, 2026 08:42
98d0480 to
f3243fa
Compare
Member
Author
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
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.
Outcome
Ownership transfer events now follow the transaction boundary established by the atomic store contract. When a transfer joins an ambient transaction, both membership update events are registered through
publishAfterCommit; when the store operation commits independently, the existing typed no-active-transaction signal falls back to immediate publication. Other publication errors remain visible.This is the remaining event-timing delta on top of #1481, which introduced the atomic in-memory and Drizzle ownership contracts and concurrency enforcement.
Fixes #1464
Verification
@croco/membership-core— 79/79 tests