chore(worker): Refactor permission syncing join table to be between Account <> Repo #600
Conversation
This comment has been minimized.
This comment has been minimized.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughMigrates permission-syncing infrastructure from user-level to account-level tracking. Renames entities (UserPermissionSyncer → AccountPermissionSyncer, UserToRepoPermission → AccountToRepoPermission, UserPermissionSyncJob → AccountPermissionSyncJob), updates database schema, and refactors data models and integration logic across backend and web packages. Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as Permission Scheduler
participant Syncer as AccountPermissionSyncer
participant DB as Database
participant GitLab/GitHub as Git Provider
Note over Scheduler,DB: New Account-Based Permission Sync
Scheduler->>Syncer: schedulePermissionSync(accounts)
Syncer->>DB: Create AccountPermissionSyncJob entries
Syncer->>Syncer: startScheduler()
loop For Each AccountPermissionSyncJob
Syncer->>DB: Load account with user data
Syncer->>Syncer: Group repos by account.provider
Syncer->>GitLab/GitHub: Fetch permissions for account
Syncer->>DB: Update account.permissionSyncedAt
Syncer->>DB: Upsert AccountToRepoPermission records
Syncer->>DB: Update AccountPermissionSyncJob status
end
Note over Syncer,DB: On Completion/Failure
Syncer->>DB: Mark job as COMPLETED/FAILED
Syncer->>Syncer: Log with account context
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.md(1 hunks)packages/backend/src/ee/accountPermissionSyncer.ts(8 hunks)packages/backend/src/ee/repoPermissionSyncer.ts(4 hunks)packages/backend/src/index.ts(4 hunks)packages/db/prisma/migrations/20251105021913_move_permission_syncing_to_account_level/migration.sql(1 hunks)packages/db/prisma/schema.prisma(3 hunks)packages/web/src/prisma.ts(2 hunks)packages/web/src/withAuthV2.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*
📄 CodeRabbit inference engine (.cursor/rules/style.mdc)
Filenames should always be camelCase. Exception: if there are filenames in the same directory with a format other than camelCase, use that format to keep things consistent.
Files:
packages/db/prisma/migrations/20251105021913_move_permission_syncing_to_account_level/migration.sqlpackages/backend/src/ee/repoPermissionSyncer.tspackages/web/src/prisma.tspackages/backend/src/ee/accountPermissionSyncer.tspackages/backend/src/index.tspackages/db/prisma/schema.prismaCHANGELOG.mdpackages/web/src/withAuthV2.ts
🪛 GitHub Actions: Test Web
packages/web/src/withAuthV2.ts
[error] 91-91: TypeError: Cannot read properties of undefined (reading 'map') at Module.getAuthContext (src/withAuthV2.ts:91). This occurs during test run of 'yarn workspace @sourcebot/web test' with 18 failing tests in withAuthV2.*
🪛 GitHub Check: build
packages/web/src/withAuthV2.ts
[failure] 91-91: src/withAuthV2.test.ts > withOptionalAuthV2 > should call the callback with the auth context object if a valid session is present and the user is a member of the organization
TypeError: Cannot read properties of undefined (reading 'map')
❯ getAuthContext src/withAuthV2.ts:91:30
❯ Module.withOptionalAuthV2 src/withAuthV2.ts:44:25
❯ src/withAuthV2.test.ts:483:24
[failure] 91-91: src/withAuthV2.test.ts > withAuthV2 > should return a service error if the user is not a member of the organization (guest role)
TypeError: Cannot read properties of undefined (reading 'map')
❯ getAuthContext src/withAuthV2.ts:91:30
❯ Module.withAuthV2 src/withAuthV2.ts:28:25
❯ src/withAuthV2.test.ts:458:24
[failure] 91-91: src/withAuthV2.test.ts > withAuthV2 > should return a service error if the user is a guest of the organization
TypeError: Cannot read properties of undefined (reading 'map')
❯ getAuthContext src/withAuthV2.ts:91:30
❯ Module.withAuthV2 src/withAuthV2.ts:28:25
❯ src/withAuthV2.test.ts:439:24
[failure] 91-91: src/withAuthV2.test.ts > withAuthV2 > should call the callback with the auth context object if a valid session is present and the user is a member of the organization with OWNER role (api key)
TypeError: Cannot read properties of undefined (reading 'map')
❯ getAuthContext src/withAuthV2.ts:91:30
❯ Module.withAuthV2 src/withAuthV2.ts:28:25
❯ src/withAuthV2.test.ts:386:24
[failure] 91-91: src/withAuthV2.test.ts > withAuthV2 > should call the callback with the auth context object if a valid session is present and the user is a member of the organization (api key)
TypeError: Cannot read properties of undefined (reading 'map')
❯ getAuthContext src/withAuthV2.ts:91:30
❯ Module.withAuthV2 src/withAuthV2.ts:28:25
❯ src/withAuthV2.test.ts:351:24
[failure] 91-91: src/withAuthV2.test.ts > withAuthV2 > should call the callback with the auth context object if a valid session is present and the user is a member of the organization with OWNER role
TypeError: Cannot read properties of undefined (reading 'map')
❯ getAuthContext src/withAuthV2.ts:91:30
❯ Module.withAuthV2 src/withAuthV2.ts:28:25
❯ src/withAuthV2.test.ts:316:24
[failure] 91-91: src/withAuthV2.test.ts > withAuthV2 > should call the callback with the auth context object if a valid session is present and the user is a member of the organization
TypeError: Cannot read properties of undefined (reading 'map')
❯ getAuthContext src/withAuthV2.ts:91:30
❯ Module.withAuthV2 src/withAuthV2.ts:28:25
❯ src/withAuthV2.test.ts:286:24
[failure] 91-91: src/withAuthV2.test.ts > getAuthContext > should return a auth context object if a valid session is present and the user is not a member of the organization. The role should be GUEST.
TypeError: Cannot read properties of undefined (reading 'map')
❯ Module.getAuthContext src/withAuthV2.ts:91:30
❯ src/withAuthV2.test.ts:237:29
[failure] 91-91: src/withAuthV2.test.ts > getAuthContext > should return a auth context object if a valid session is present and the user is a member of the organization with OWNER role
TypeError: Cannot read properties of undefined (reading 'map')
❯ Module.getAuthContext src/withAuthV2.ts:91:30
❯ src/withAuthV2.test.ts:212:29
[failure] 91-91: src/withAuthV2.test.ts > getAuthContext > should return a auth context object if a valid session is present and the user is a member of the organization
TypeError: Cannot read properties of undefined (reading 'map')
❯ Module.getAuthContext src/withAuthV2.ts:91:30
❯ src/withAuthV2.test.ts:182:29
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (8)
packages/db/prisma/schema.prisma (1)
62-62: LGTM! Schema migration is well-structured.The refactoring from user-level to account-level permission tracking is implemented correctly:
- Relationships properly updated (permittedUsers → permittedAccounts)
- Foreign keys with appropriate cascade rules
- Composite primary key on AccountToRepoPermission ensures uniqueness
Also applies to: 365-395, 412-416
CHANGELOG.md (1)
21-26: LGTM! Documentation accurately describes the change.The CHANGELOG entry clearly documents the internal representation change from user-based to account-based permissions.
packages/backend/src/index.ts (1)
14-14: LGTM! Syncer references updated consistently.All references from
UserPermissionSyncertoAccountPermissionSyncerare properly updated across import, instantiation, and lifecycle methods.Also applies to: 55-55, 68-68, 84-84
packages/web/src/prisma.ts (1)
23-64: LGTM! Prisma extension correctly updated for account-based scoping.The function signature and query logic are properly updated to:
- Accept an array of
accountIdsinstead of a singleuserId- Use the
inoperator for filtering against multiple accounts- Reference
permittedAccountsinstead ofpermittedUserspackages/backend/src/ee/repoPermissionSyncer.ts (1)
171-247: LGTM! Repo permission syncer correctly refactored for accounts.The changes properly:
- Map collaborators/members to account IDs instead of user IDs
- Update the relation from
permittedUserstopermittedAccounts- Use
accountIdin theaccountToRepoPermissionrecordspackages/db/prisma/migrations/20251105021913_move_permission_syncing_to_account_level/migration.sql (1)
1-65: Verify migration and data loss implications.The migration correctly implements the account-level permission model, but note:
- Old data in
UserPermissionSyncJobandUserToRepoPermissionwill be lost- PR description indicates "TODO: test migration path"
Ensure the migration path is tested before merging, especially if there's existing production data.
Consider:
- Testing the migration on a copy of production data
- Documenting the data loss implications for users
- Potentially creating a data migration script to preserve historical permission records if needed
packages/backend/src/ee/accountPermissionSyncer.ts (1)
2-2: LGTM! Account permission syncer is comprehensively refactored.The refactoring from
UserPermissionSyncertoAccountPermissionSynceris thorough and correct:
- Database operations properly updated to use account entities
- Provider-based filtering aligns with account model
- Logging context updated to account-centric messages
- Transaction logic correctly maps repos to accounts
Also applies to: 17-25, 54-98, 110-130, 136-237, 240-303
packages/web/src/withAuthV2.ts (1)
110-114: No issues found - all user queries correctly include accounts relation.Verification confirms both
user.findUniquecalls in the file (lines 107-114 and 128-135) includeaccounts: true. The accounts relation is consistently loaded across all code paths that fetch users, and line 91 demonstrates the relation is actively used in the code.
This PR refactors the
UserToRepoPermissionjoin table into a new table,AccountToRepoPermission, that maps Accounts and Repos (instead of Users and Repos). The backstory here is that we are adding support for account linking where users will be able to link multiple accounts (e.g., their GitHub, GitLab, and Okta accounts). For various reasons (including linking & unlinking of Accounts), having the join table be at the Account <> Repo level makes our lives easier.Summary by CodeRabbit