Repository navigation
fix(web/ee): load identity provider config once in getLinkedAccounts - #1602
Harsh23Kashyap wants to merge 1 commit into
Conversation
`getLinkedAccounts` called `getIdentityProviderConfig(account.providerId)` inside a for-loop over the user's linked accounts. Each call internally re-invokes `getIdentityProviderConfigs` (in `packages/shared/src/env.server.ts`), which re-reads the config file from disk (or re-fetches the remote config URL) and re-validates it with ajv on every iteration. A user with N linked accounts triggered N full config reads on every call to `getLinkedAccounts`, plus one more read from the second loop in the same function. With a 50 KB config file and 5 linked accounts, that is 5 x 50 KB of disk reads + 5 x JSON.parse + 5 x ajv schema validation per settings-page load. The retry-on-`ENOENT` logic in `loadConfig` (5 attempts x 2 s delay) also amplified the worst-case latency when the config file was being mounted. Hoist the `getIdentityProviderConfigs()` call to once before the first loop, mirror the pattern already used by the second loop, and use a synchronous object lookup per account. The second loop now reuses the same map. Other call sites of `getIdentityProviderConfig` are single-call (`backend/src/ee/accountPermissionSyncer.ts:292`, `backend/src/ee/tokenRefresh.ts:213`, `web/src/lib/encryptedPrismaAdapter.ts:31`) and are not in a loop, so the N+1 does not apply to them. They are out of scope here. Add two vitest cases to `actions.test.ts`: - one that exercises the merge of linked and unlinked account_linking providers and pins the result shape, and - one that asserts `getIdentityProviderConfigs` is called exactly once for five linked accounts (regression for the N+1). Fixes #1601
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan includes up to 8 reviews per rolling hour; 7 remain after this review. Walkthrough
ChangesLinked account configuration reuse
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This localized change reuses the identity-provider configuration during linked-account processing, reducing repeated config reads and validation without introducing an actionable merge-blocking risk; it is merge-ready after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Summary
getLinkedAccountscalledgetIdentityProviderConfig(account.providerId)inside a for-loop over the user's linked accounts. Each call internally re-invokesgetIdentityProviderConfigs(inpackages/shared/src/env.server.ts), which re-reads the config file from disk (or re-fetches the remote config URL) and re-validates it with ajv on every iteration.A user with N linked accounts triggered N full config reads on every call to
getLinkedAccounts, plus one more read from the second loop in the same function. With a 50 KB config file and 5 linked accounts, that is 5 × 50 KB of disk reads + 5 ×JSON.parse+ 5 × ajv schema validation per settings-page load. The retry-on-ENOENTlogic inloadConfig(5 attempts × 2 s delay) also amplified the worst-case latency when the config file was being mounted.Hoist the
getIdentityProviderConfigs()call to once before the first loop, mirror the pattern already used by the second loop, and use a synchronous object lookup per account. The second loop now reuses the same map.Validation
yarn workspace @sourcebot/web test --run src/ee/features/sso/actions.test.ts→ 6 passed (4 pre-existing + 2 new)yarn workspace @sourcebot/web test --run→ 1168 passed (same 7 pre-existing@opentelemetry/sdk-trace-basesuite-load failures asupstream/main)yarn workspace @sourcebot/web lint→ cleanyarn workspace @sourcebot/web exec tsc --noEmit→ 0 new errors introduced (the existingwithAuth.tsSCOPED_ACCESS_TOKEN_PREFIX/scopedAccessTokenerrors are pre-existing onupstream/main@39b5953band unrelated to this PR)Test plan
yarn workspace @sourcebot/web test --run src/ee/features/sso/actions.test.ts(6 passed: 4 pre-existing + 2 new)yarn workspace @sourcebot/web test --run(1168 passed; same 7 pre-existing suite-load failures asupstream/main)yarn workspace @sourcebot/web lint(0 errors)npx tsc --noEmit -p packages/web/tsconfig.json(0 new errors)Fixes #1601
Note
Cursor Bugbot is generating a summary for commit c5cf7a2. Configure here.
Summary by CodeRabbit