Batch Repo addition to a team(Backend) #672
Conversation
WalkthroughA new API endpoint is introduced to fetch repositories from GitHub or GitLab organizations, supporting pagination and selecting all repositories. Token retrieval logic is centralized into utility functions. Tests are added for the new endpoint and updated for GitHub URL utilities. Minor configuration and export adjustments are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Handler
participant Utils
participant GitHub_API
participant GitLab_API
participant DB
Client->>API_Handler: GET /api/internal/[org_id]/git_org_repos
API_Handler->>Utils: getGithubToken/org_id or getGitlabToken/org_id
Utils->>DB: Query Integration table
DB-->>Utils: Return encrypted token
Utils->>Utils: Decrypt token
Utils-->>API_Handler: Return token
alt provider is GitHub
API_Handler->>GitHub_API: GraphQL query for repos (with pagination)
GitHub_API-->>API_Handler: Repo data
else provider is GitLab
API_Handler->>GitLab_API: GraphQL query for repos (with pagination)
GitLab_API-->>API_Handler: Repo data
end
API_Handler-->>Client: Return repos (paginated or all)
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
thesujai
changed the title
thesujai
force-pushed
the
repos-import-bulk
branch
from
ce98b9d to
eac96ef
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (2)
web-server/pages/api/internal/[org_id]/git_provider_org.ts (1)
30-36:tokens.shift()insidemapobscures intent and can mis-align provider/token pairs
Promise.all(providerMap.map(item => item.search(tokens.shift(), …)))relies on mutation and positional coupling.
If any earliergetTokenresolves toundefined(integration missing, decrypt failure), the shift will silently slide the array, pairing the wrong token with the next provider.-const repos = await Promise.all( - providerMap.map((item) => item.search(tokens.shift(), search_text)) -); +const repos = await Promise.all( + providerMap.map((item, idx) => item.search(tokens[idx], search_text)) +);Clearer, O(1), and robust to sparse elements.
web-server/pages/api/internal/[org_id]/utils.ts (1)
291-307:replaceURLignoresorg_id– custom domain might clash across orgsThe query filters only by
name = GITLAB, so if multiple orgs store differentcustom_domainvalues the first row wins.
Either addorg_idto thewhereclause or document that the GitLab domain is global.-const provider_meta = await db('Integration') - .where('name', Integration.GITLAB) +const provider_meta = await db('Integration') + .where({ name: Integration.GITLAB, org_id })(Requires passing
org_idinto the helper.)
🧹 Nitpick comments (3)
web-server/package.json (1)
132-135: Node 22 × Yarn 1 combo – double-check CI image and local dev setupDeclaring
"engines": { "node": "^22.x" }while locking the project to Yarn 1 ("packageManager": "yarn@1.22.22") is fine syntactically, but Node 22 is still experimental in many standard build images and some Yarn 1 plugins (esp.node-gypconsumers) lag behind new major Node versions.
Make sure the CI/Prod containers actually ship Node 22, or relax the engine range to the latest LTS (e.g.^20.0.0) to avoid accidental install failures (npm ERR! Unsupported engine).web-server/pages/api/internal/[org_id]/__tests__/github.test.ts (1)
78-82: Minor nit – duplicatehttps://stripped twice
getGitHubGraphQLUrlalready normalises slashes; no action needed, but future tests could drop the trailing/apirepetition (https://api.github.local/api/graphql) to focus on the important part.web-server/pages/api/internal/[org_id]/__tests__/git_org_repos.test.ts (1)
16-18: Prefer resettingglobal.fetchwith assignment overdeleteStatic analysis rightly flags
delete (global as any).fetch; it slows objects on V8 and is unnecessary in test code.-// @ts-ignore -delete (global as any).fetch +// @ts-expect-error – test shim +(global as any).fetch = undefined
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
web-server/package.json(1 hunks)web-server/pages/api/internal/[org_id]/__tests__/git_org_repos.test.ts(1 hunks)web-server/pages/api/internal/[org_id]/__tests__/github.test.ts(2 hunks)web-server/pages/api/internal/[org_id]/git_org_repos.ts(1 hunks)web-server/pages/api/internal/[org_id]/git_provider_org.ts(1 hunks)web-server/pages/api/internal/[org_id]/utils.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
web-server/pages/api/internal/[org_id]/__tests__/git_org_repos.test.ts (2)
web-server/pages/api/internal/[org_id]/git_org_repos.ts (2)
fetchRepos(31-199)selectAllRepos(201-220)backend/analytics_server/mhq/store/models/integrations/integrations.py (1)
Integration(10-28)
web-server/pages/api/internal/[org_id]/utils.ts (2)
backend/analytics_server/mhq/store/models/integrations/integrations.py (1)
Integration(10-28)web-server/src/utils/auth-supplementary.ts (1)
dec(20-25)
web-server/pages/api/internal/[org_id]/git_org_repos.ts (3)
web-server/src/api-helpers/global.ts (1)
Endpoint(26-106)web-server/pages/api/internal/[org_id]/utils.ts (4)
getGitHubGraphQLUrl(335-338)replaceURL(291-308)getGithubToken(340-350)getGitlabToken(352-362)web-server/src/types/resources.ts (1)
BaseRepo(242-254)
🪛 Biome (1.9.4)
web-server/pages/api/internal/[org_id]/__tests__/git_org_repos.test.ts
[error] 16-16: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 126-126: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (5)
web-server/pages/api/internal/[org_id]/git_provider_org.ts (1)
45-56: Great improvement – token retrieval logic is now fully centralised
ImportinggetGithubToken/getGitlabTokenfrom the utils module removes duplication from this handler and keeps secrets logic in one place.web-server/pages/api/internal/[org_id]/__tests__/github.test.ts (1)
58-68: 👍 Scheme-prefixed custom domain mocks align with production helperThe tests now use full URLs (
https://git.sujai.com) which mirrors the format expected bygetGitHubCustomDomain(). Assertions remain correct.web-server/pages/api/internal/[org_id]/__tests__/git_org_repos.test.ts (1)
129-145: Nice coverage – confirms pagination loop terminates correctly
The two-page mock andexpect(fetchMock).toHaveBeenCalledTimes(2)accurately guard against infinite loops or premature exits inselectAllRepos().web-server/pages/api/internal/[org_id]/utils.ts (1)
323-333: Good: URL de-duplication helper keeps consumer code clean
The negative-look-behind regex for double slashes is concise and safe.web-server/pages/api/internal/[org_id]/git_org_repos.ts (1)
81-84: Use proper “Bearer” scheme in Authorization headerOAuth2 spec (and GitHub examples) capitalise the scheme:
Authorization: Bearer <token>Some servers are case-sensitive; using lower-case “bearer” risks 401s.
-Authorization: `bearer ${token}`, +Authorization: `Bearer ${token}`,
web-server/pages/api/internal/[org_id]/__tests__/git_org_repos.test.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
web-server/pages/api/internal/[org_id]/__tests__/git_org_repos.test.ts (1)
125-127: Duplicate: avoiddelete global.fetchSame recommendation as on lines 15-17—assign
undefinedinstead of usingdelete.
🧹 Nitpick comments (3)
web-server/pages/api/internal/[org_id]/__tests__/git_org_repos.test.ts (3)
15-17: Replacedelete global.fetchwith assignmentDeleting a property on
globalis slower and triggers thenoDeletelint rule. You can achieve the same reset with a simple assignment.-// @ts-ignore -delete (global as any).fetch +// @ts-expect-error – reset the polyfill between tests +(global as any).fetch = undefined
6-9: Align import indentationThe extra left-padding makes these top-level imports look nested inside the previous block. Align them to column 0 for consistency.
- import { fetchRepos, selectAllRepos } from '@/api/internal/[org_id]/git_org_repos' - import { Integration } from '@/constants/integrations' - import * as utils from '@/api/internal/[org_id]/utils' +import { fetchRepos, selectAllRepos } from '@/api/internal/[org_id]/git_org_repos' +import { Integration } from '@/constants/integrations' +import * as utils from '@/api/internal/[org_id]/utils'
12-18: Add negative-path tests for complete coverage
fetchReposthrows whenres.okisfalseor when a GraphQLerrorsarray is present, but the suite only covers the happy path. A couple of extra test cases with:
global.fetchresolving to{ ok: false, status: 500 }global.fetchresolving to{ ok: true, json: () => ({ errors: [{ message: 'boom' }] }) }would lock in the error-handling behaviour and protect against regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web-server/pages/api/internal/[org_id]/__tests__/git_org_repos.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
web-server/pages/api/internal/[org_id]/__tests__/git_org_repos.test.ts (1)
web-server/pages/api/internal/[org_id]/git_org_repos.ts (2)
fetchRepos(31-199)selectAllRepos(201-220)
🪛 Biome (1.9.4)
web-server/pages/api/internal/[org_id]/__tests__/git_org_repos.test.ts
[error] 16-16: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 126-126: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: All file linting
🔇 Additional comments (1)
web-server/pages/api/internal/[org_id]/__tests__/git_org_repos.test.ts (1)
115-119: 👍 GitLab matcher now reflects the real object shapeThe assertion has been updated to use
slug,web_url, andparent, matching whatfetchReposactually returns for GitLab repos. This addresses the previous review’s concern and prevents false negatives.
Linked Issue(s)
Fixes part of #664
Proposed changes (including videos or screenshots)
git_provider_orgso that they can be re-used for the new api endpointFurther comments
There was a minor issue with the github utils test, that is fixed with this PR
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores