Skip to content

fix: add/remove connected repos in DB on itegration update#65

Open
OpeOginni wants to merge 9 commits into
Boring-Software-Inc:mainfrom
OpeOginni:fix/github-integration
Open

fix: add/remove connected repos in DB on itegration update#65
OpeOginni wants to merge 9 commits into
Boring-Software-Inc:mainfrom
OpeOginni:fix/github-integration

Conversation

@OpeOginni

@OpeOginni OpeOginni commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Summary

amber-harbor-213

  • Listens to Github Installation Updates to decide which new repos are added and which ones to remove
  • Adds Disclamer Dialog in frontend letting user know that removing a repo deletes all data collected realted to it.

Checklist

Notes for reviewers

Screen.Recording.2026-06-24.at.10.30.29.mov

Summary by CodeRabbit

  • New Features

    • Added modal dialogs for managing GitHub repository access and confirming disconnections.
    • Added support for synchronizing connected GitHub repositories when installations are created or updated.
    • GitHub installation updates now refresh repository access automatically.
  • Bug Fixes

    • Improved handling of GitHub repository additions and removals from webhook events.
    • Ensured repository access stays aligned with the current GitHub installation.

@vercel

vercel Bot commented Jun 24, 2026

Copy link
Copy Markdown

@OpeOginni is attempting to deploy a commit to the Bounty Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

GitHub installation handling now fetches and synchronizes repository records for both new and existing organizations. Callback processing accepts installation updates, while repository webhooks process added and removed repositories directly from payload arrays. The integrations page replaces inline uninstall confirmation with Manage GitHub repos and uninstall modal dialogs.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title matches the main change: syncing connected repositories in the database during GitHub integration updates.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ripgrim

ripgrim commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

My recent PR introduced a new integrations UI, could you fetch from origin and add this again? @OpeOginni I love this PR a lot, looking forward to merging another banger from you!

@OpeOginni

Copy link
Copy Markdown
Contributor Author

@ripgrim Just added a dialog to both the manage and uninstall button letting users know it clears data related to those repo.

The extra changes were just format ones, to let the action succeed.

@ripgrim

ripgrim commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

@ripgrim Just added a dialog to both the manage and uninstall button letting users know it clears data related to those repo.

The extra changes were just format ones, to let the action succeed.

any chance you could toss in a little screen recording or screenshots? Will review this tomorrow!

@OpeOginni

Copy link
Copy Markdown
Contributor Author
Trimmed.mov

Here we go

@OpeOginni

Copy link
Copy Markdown
Contributor Author

Small bump @ripgrim

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
apps/web/src/lib/github/webhook.ts (1)

176-198: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Dropping the payload.action gate and iterating repositories_added ?? [] / repositories_removed ?? [] is more robust since only the relevant array is populated per event. Minor: the added loop is N+1 (per-repo select then insert); consider a batched insert ... onConflictDoNothing and a single delete ... where inArray(githubRepoId, removedIds) if bulk add/remove events are expected.

🤖 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 `@apps/web/src/lib/github/webhook.ts` around lines 176 - 198, Optimize
repository change handling in the added and removed loops: replace
per-repository existence checks and inserts with a batched insert using
onConflictDoNothing, and replace repeated deletes with one delete constrained by
inArray over the removed repository IDs. Preserve the existing org and
repository field mappings and logging behavior where practical.
apps/web/src/lib/github/install.ts (1)

118-152: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy lift

applyRepoSync is non-transactional and issues N+1 queries; also drops metadata updates.

Two concerns on top of the pagination issue flagged above:

  • The insert-missing and delete-stale phases run as separate, un-batched statements outside a transaction. A mid-sync failure leaves the DB in a partial state, and the per-repo select (Line 125-128) is N+1. Wrap the sync in db.transaction(...) and batch the delete via inArray/notInArray.
  • Existing repos are never updated, so a rename (name/full_name) or visibility change (private) on GitHub never propagates. Consider onConflictDoUpdate on githubRepoId.
🤖 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 `@apps/web/src/lib/github/install.ts` around lines 118 - 152, Refactor
applyRepoSync to execute atomically inside db.transaction, replace the
per-repository existence selects and individual stale deletes with batched
operations using onConflictDoUpdate and inArray/notInArray, and update existing
records’ name, fullName, and isPrivate fields when GitHub metadata changes.
Preserve the stale-repository cleanup while ensuring all inserts, updates, and
deletes complete as one transaction.
🤖 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 `@apps/web/src/components/layout/app/integrations/integrations-page.tsx`:
- Around line 200-273: Replace the hardcoded border-[`#27272A`] class with
border-tw-border on both DialogClose Cancel buttons in the manageDialogOpen and
confirmingId dialogs, matching the file’s existing design-token styling.

In `@apps/web/src/lib/github/install.ts`:
- Around line 93-116: Update fetchInstallationRepos to retrieve all pages from
the GitHub installation repositories endpoint before returning. Iterate using
the response pagination metadata or page parameters, accumulate every repository
into one array, preserve error handling for failed requests, and return the
complete collection so applyRepoSync does not remove repositories beyond the
first 100.

---

Nitpick comments:
In `@apps/web/src/lib/github/install.ts`:
- Around line 118-152: Refactor applyRepoSync to execute atomically inside
db.transaction, replace the per-repository existence selects and individual
stale deletes with batched operations using onConflictDoUpdate and
inArray/notInArray, and update existing records’ name, fullName, and isPrivate
fields when GitHub metadata changes. Preserve the stale-repository cleanup while
ensuring all inserts, updates, and deletes complete as one transaction.

In `@apps/web/src/lib/github/webhook.ts`:
- Around line 176-198: Optimize repository change handling in the added and
removed loops: replace per-repository existence checks and inserts with a
batched insert using onConflictDoNothing, and replace repeated deletes with one
delete constrained by inArray over the removed repository IDs. Preserve the
existing org and repository field mappings and logging behavior where practical.
🪄 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: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 67abbc8f-fcd0-44b6-9582-3b03f2c2a923

📥 Commits

Reviewing files that changed from the base of the PR and between 2aef301 and ac24861.

📒 Files selected for processing (4)
  • apps/web/src/components/layout/app/integrations/integrations-page.tsx
  • apps/web/src/lib/github/install.ts
  • apps/web/src/lib/github/webhook.ts
  • apps/web/src/routes/api/github/callback.ts

Comment thread apps/web/src/lib/github/install.ts
Paginate installation repository fetches and use design tokens on dialog Cancel buttons.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants