Reconcile displayed roles after the writes, not between them - #10
Open
DJAscendance wants to merge 2 commits into
Open
Reconcile displayed roles after the writes, not between them#10DJAscendance wants to merge 2 commits into
DJAscendance wants to merge 2 commits into
Conversation
reconcilePrimaryRole reads role_assignment to decide whether a member's displayed role is still one they hold. The callers ran it between the remove and the add that replaced it, so it observed a state that never settled. The simplest case needs no transfer at all. postAccessInfo removed the owner's assignment, reconciled, and only then re-added it -- so re-saving an access page WITHOUT changing the owner cleared that owner's primary_role_id, because at the moment of the read they genuinely held nothing. They kept the role and lost the badge, and nothing in the request explained why. Editing a place's deputies was enough to strip its owner's displayed role. Every mutation now lands first and reconciliation happens once at the end: - syncDeputies collects the members it removed instead of reconciling inside the remove loop, and reconciles them after the add loop. - Given a collector, it defers to that instead, so a caller changing both the owner and the deputies reconciles the whole set once at the very end rather than once per axis. - reconcilePrimaryRoles is that final pass. It deduplicates, because the same member can be touched on more than one axis of one update -- losing a deputy slot while also being the outgoing owner -- and skips the 0 sentinel. - block, hood, colony and place collect the outgoing owner into that set rather than reconciling inline. place.service's deputy-less guard became a conditional instead of an early return. 'jail' and 'cityhall' have an owner role and no deputy role, and returning there would have skipped the owner's reconciliation along with the deputy sync -- reintroducing the same bug for exactly the two places the guard exists for. Verified: 7 new tests, 24 total on this service. The ordering guarantee is asserted directly via mock invocationCallOrder -- the first getPrimaryRoleId read must come after the last addIdToAssignment -- which fails on the previous code, where reconciliation ran inside the remove loop before any add. Plus the collector path, a member who merely changed position not being collected, dedupe, sentinel skipping, and continuing past a member that throws. tsc clean apart from the pre-existing missing 'sharp' module. eslint 0 errors. Full suite compared against a stashed baseline test-name by test-name: identical failures, no regressions, 55 -> 62 passing. Deliberately not done: this fixes ORDERING, not atomicity. The writes are still separate statements, so a concurrent request interleaving between them can still observe a gap. Closing that needs a knex transaction threaded from the service through RoleAssignmentRepository, which today takes no trx and reaches for this.db.knex directly -- a repository-wide signature change well beyond this fix. The ordering bug was the one that fired on a single request with no concurrency at all.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAccess-information updates now defer primary-role reconciliation until owner and deputy assignment changes finish. Role assignment synchronization collects affected members, deduplicates reconciliation, isolates errors, and supports places without deputy roles. ChangesPrimary-role reconciliation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AccessInfoService
participant RoleAssignmentService
participant role_assignment
participant primary_role_id
AccessInfoService->>RoleAssignmentService: syncDeputies(..., touched)
RoleAssignmentService->>role_assignment: Remove and add deputy assignments
RoleAssignmentService->>AccessInfoService: Collect affected member IDs
AccessInfoService->>RoleAssignmentService: reconcilePrimaryRoles(touched)
RoleAssignmentService->>primary_role_id: Reconcile each touched member
Possibly related PRs
🚥 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 |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/src/services/block/block.service.ts (1)
87-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the shared owner-swap/deputy-sync/reconcile sequence instead of duplicating it four times. All four
postAccessInfomethods independently re-implement the identicaltouchedset + remove-old-owner + add-new-owner +syncDeputies(..., touched)+reconcilePrimaryRoles(touched)sequence. This is exactly the kind of duplication that let the original bug (and, perrole-assignment.service.ts's docstring, its admin.service.ts sibling) go unfixed in one place while being fixed elsewhere. Moving this sequence into a single shared method (e.g. onRoleAssignmentService, taking placeId/ownerCode/deputyCode/oldOwner/newOwner/deputies) would make future ordering fixes apply everywhere at once.
api/src/services/block/block.service.ts#L87-L106: replace the inline touched/remove/add/sync sequence with a call to the shared helper.api/src/services/colony/colony.service.ts#L83-L102: same.api/src/services/hood/hood.service.ts#L82-L101: same.api/src/services/place/place.service.ts#L257-L285: same, passing through the deputyCode-optional branch.🤖 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 `@api/src/services/block/block.service.ts` around lines 87 - 106, Extract the shared owner-swap, deputy synchronization, and primary-role reconciliation sequence into one helper on RoleAssignmentService, accepting the place ID, owner/deputy codes, old and new owners, and deputy data. Replace the duplicated inline sequence in api/src/services/block/block.service.ts:87-106, api/src/services/colony/colony.service.ts:83-102, and api/src/services/hood/hood.service.ts:82-101 with calls to that helper; update api/src/services/place/place.service.ts:257-285 likewise while preserving its optional deputyCode branch.
🤖 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.
Nitpick comments:
In `@api/src/services/block/block.service.ts`:
- Around line 87-106: Extract the shared owner-swap, deputy synchronization, and
primary-role reconciliation sequence into one helper on RoleAssignmentService,
accepting the place ID, owner/deputy codes, old and new owners, and deputy data.
Replace the duplicated inline sequence in
api/src/services/block/block.service.ts:87-106,
api/src/services/colony/colony.service.ts:83-102, and
api/src/services/hood/hood.service.ts:82-101 with calls to that helper; update
api/src/services/place/place.service.ts:257-285 likewise while preserving its
optional deputyCode branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: adb65224-71d9-4cd5-a30c-dd862bafe780
📒 Files selected for processing (6)
api/src/services/block/block.service.tsapi/src/services/colony/colony.service.tsapi/src/services/hood/hood.service.tsapi/src/services/place/place.service.tsapi/src/services/role-assignment/role-assignment.service.spec.tsapi/src/services/role-assignment/role-assignment.service.ts
CodeRabbit review of #10. The previous commit fixed the reconciliation ordering but left the corrected sequence -- collect touched, remove old owner, add new owner, syncDeputies deferring into the set, reconcile the set -- written out four times, once per place type. As the review put it, that is the same duplication that let the original bug go unfixed in one place while being fixed elsewhere. It was four chances to get an order-sensitive sequence subtly wrong, in code where getting it wrong is invisible: the request succeeds and a member quietly loses their badge. RoleAssignmentService.syncPlaceAccess now owns the order. Callers keep what is genuinely theirs -- which role ids count as owner and deputy there, and resolving submitted usernames to member ids -- and hand over the writes. place.service keeps a local check for whether the place has a deputy role at all, but only to skip pointless username lookups for 'jail' and 'cityhall'. The skip-the-deputy-half decision itself is the shared method's, so those places still get their owner swapped and reconciled. 6 new tests, 30 on this service. The headline one asserts the original bug directly: with the owner UNCHANGED, the first getPrimaryRoleId read must come after the last addIdToAssignment, via mock invocationCallOrder. Also covered: the outgoing owner reconciled after the incoming one is written, no owner writes when the place has none on either side, a deputy-less place still swapping and reconciling its owner, a member who is both outgoing owner and dropped deputy being reconciled exactly once, and both halves applying together. Verified: tsc CLEAN -- the missing 'sharp' module that failed on every branch all session was a genuinely absent dependency, declared in package.json and package-lock.json since the home-image work but never installed into the shared api/node_modules. Installed with --no-save so the tracked lockfile is untouched; every worktree symlinks that directory so all five branches typecheck clean now. Nothing else was hiding behind that error. eslint 0 errors. Suite 62 -> 76 passing, same 2 pre-existing failures.
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.
Stacked on #9 (
fix/deputy-reconciliation) — review that first; the diff here is only the last commit.Closes the fifth finding from the CodeRabbit App review of #6, which was deliberately deferred there because it changes when every caller reconciles.
The bug
reconcilePrimaryRolereadsrole_assignmentto decide whether a member's displayed role is still one they hold. The callers ran it between the remove and the add that replaced it, so it observed a state that never settled.The simplest failing case involves no transfer at all — just re-saving an access page without touching the owner:
role_assignmentreconcilePrimaryRole(owner)primary_role_idThe owner keeps the role and silently loses their displayed role. Editing a place's deputies was enough to strip that place's owner of their badge, with nothing in the request to explain it.
The fix
Every mutation lands first; reconciliation happens once at the end.
syncDeputiescollects the members it removed rather than reconciling inside the remove loop, and reconciles after the add loop.reconcilePrimaryRolesis that final pass. It deduplicates, because one member can be touched on more than one axis of a single update (losing a deputy slot while also being the outgoing owner), and skips the0sentinel.block,hood,colonyandplacecollect the outgoing owner into that set instead of reconciling inline.place.service's deputy-less guard became a conditional rather than an earlyreturn.jailandcityhallhave an owner role and no deputy role, and returning there would have skipped the owner's reconciliation along with the deputy sync — reintroducing this exact bug for precisely the two places the guard exists for.Verification
7 new tests, 24 on this service. The ordering guarantee is asserted directly via
mock.invocationCallOrder: the firstgetPrimaryRoleIdread must come after the lastaddIdToAssignment. That assertion fails on the previous code, where reconciliation ran inside the remove loop before any add — so it is a real regression test, not a restatement of the new implementation.Also covered: the collector path, a member who merely changed slot position not being collected at all, dedupe, sentinel skipping, and continuing past a member that throws.
tscclean apart from the pre-existing missingsharpmodule.eslint0 errors. Full suite compared against the base test-name by test-name rather than by count: identical failures, no regressions, 55 → 62 passing.Not done here
This fixes ordering, not atomicity. The writes are still separate statements, so a concurrent request interleaving between them can still observe a gap. Closing that needs a knex transaction threaded from the service through
RoleAssignmentRepository, which today takes notrxand reaches forthis.db.knexdirectly — a repository-wide signature change well beyond this fix.Worth being precise about the severity split: the ordering bug fired on a single request with no concurrency at all, which is why it was worth separating.
Summary by CodeRabbit