[Abandoned] Github integration #651
Conversation
- Created github schema tables (installations, repositories, pull_requests) - Ported OAuth flow (install/callback routes) from old worktree - Ported webhook handlers for installation, repo, PR, review, and check events - Ported initial sync job for repos and PRs - Configured Electric SQL shapes for GitHub tables - Updated to use better-auth instead of Clerk
- Add GitHub env vars to API env.ts - Install Octokit packages - Fix auth import path (use @superset/auth/server) - Add proper type annotations to webhook handlers - Fix additions/deletions/changedFiles (not available in list response) - Remove duplicate exports in MonacoProvider
GitHub App uses getInstallationOctokit() which generates fresh tokens on-demand using the app's private key. No need to store tokens in DB. Changes: - Remove accessToken, tokenExpiresAt, refreshToken fields from schema - Remove token fetching from callback route - Add lastSyncedAt field for future periodic sync tracking - Update lastSyncedAt after initial sync completes
Enables real-time sync of GitHub data to desktop app's SQLite database. Changes: - Add github_repositories and github_pull_requests to AllowedTable type - Implement buildWhereClause for github_repositories (filters by installation -> org) - Implement buildWhereClause for github_pull_requests (filters by repo -> installation -> org) - Import GitHub schema tables This unblocks the desktop app from querying GitHub data offline.
📝 WalkthroughWalkthroughThis PR introduces a comprehensive GitHub App integration enabling OAuth-based authentication, webhook event processing, and data synchronization. It adds three database tables for GitHub installations, repositories, and pull requests, corresponding API routes for installation flows and background sync jobs, webhook handling infrastructure, and updates Electric SQL proxy filtering to scope GitHub data by organization. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Desktop as Desktop App
participant API as API Server
participant GitHub as GitHub App
participant DB as Database
User->>Desktop: Initiate GitHub integration
Desktop->>API: GET /github/install?organizationId=...
API->>DB: Verify user is org member
alt Not a member
API-->>Desktop: 403 Forbidden
else Valid member
API->>API: Encode state (orgId, userId)
API-->>Desktop: Redirect to GitHub install URL
Desktop->>GitHub: Redirect to GitHub App install
GitHub-->>User: Approve permissions
User->>GitHub: Complete installation
GitHub->>API: Redirect to /github/callback?installation_id=...&state=...
API->>API: Decode & validate state
API->>GitHub: Get installation client
GitHub-->>API: Octokit client ready
API->>GitHub: Fetch installation account info
GitHub-->>API: Account login, type
API->>DB: Upsert github_installations
DB-->>API: Installation record created
API->>API: Queue initial sync job via QSTash
API-->>Desktop: Redirect to success
end
Note over API,DB: Initial Sync Job (async)
API->>GitHub: Fetch all repositories
GitHub-->>API: Paginated repo list
loop Each repository
API->>DB: Upsert github_repositories
API->>GitHub: Fetch open pull requests
GitHub-->>API: Paginated PR list
loop Each pull request
API->>GitHub: Fetch CI check runs
GitHub-->>API: Check status & details
API->>DB: Upsert github_pull_requests with aggregated checksStatus
end
end
API->>DB: Update installation.lastSyncedAt
DB-->>API: Sync complete
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @apps/api/package.json:
- Around line 16-18: The package.json lists non-existent Octokit versions;
update the dependency entries for "@octokit/app", "@octokit/rest", and
"@octokit/webhooks" to the actual published versions (set "@octokit/app" to
"16.1.0", "@octokit/rest" to "22.0.0", and "@octokit/webhooks" to "13.8.1") and
then run your package manager to regenerate the lockfile so installs succeed.
In @apps/api/src/app/api/integrations/github/callback/route.ts:
- Around line 38-46: The code calls JSON.parse on the decoded state which can
throw before Zod validation; update the callback handler around the
Buffer.from(...).toString and JSON.parse so you catch any errors (invalid base64
or invalid JSON) and on error perform the same redirect used for invalid Zod
parsing (redirect to
`${env.NEXT_PUBLIC_WEB_URL}/settings/integrations?error=invalid_state`); then
continue to run stateSchema.safeParse against the parsed object (keep the parsed
variable name) when parsing succeeds.
In @apps/api/src/app/api/integrations/github/jobs/initial-sync/route.ts:
- Around line 60-62: The "installation not found" branch in the initial-sync
route returns a 200 OK by default; update the Response.json call in the
installation missing branch (the if (!installation) check in route.ts) to return
a 404 status by passing an explicit status option so the response correctly
signals "Not Found" instead of OK.
In @apps/api/src/app/api/integrations/github/webhook/route.ts:
- Around line 17-22: The code calls JSON.parse(body) directly when constructing
the webhook event (eventId/eventType/deliveryId block) which will throw on empty
or malformed bodies; wrap the parse in a try-catch (e.g., const parsed =
safeParse(body); or try { parsed = JSON.parse(body) } catch (err) { log the
error and return a 400/422 HTTP response with a clear message instead of
proceeding }) and use the parsed variable when inserting the record (payload:
parsed) so malformed payloads are handled gracefully and do not cause an
unhandled exception.
🧹 Nitpick comments (16)
apps/api/src/app/api/electric/[...path]/utils.ts (1)
71-105: Consider extracting the duplicated installation lookup.The installation lookup logic (lines 73-80) is duplicated from the
github_repositoriescase (lines 55-62). This could be extracted to a helper function.♻️ Optional: Extract helper to reduce duplication
async function getInstallationId(organizationId: string): Promise<string | null> { const installation = await db.query.githubInstallations.findFirst({ where: eq(githubInstallations.organizationId, organizationId), columns: { id: true }, }); return installation?.id ?? null; }Then use in both cases:
case "github_repositories": { - const installation = await db.query.githubInstallations.findFirst({ - where: eq(githubInstallations.organizationId, organizationId), - columns: { id: true }, - }); - - if (!installation) { + const installationId = await getInstallationId(organizationId); + if (!installationId) { return { fragment: "1 = 0", params: [] }; } - - return build( - githubRepositories, - githubRepositories.installationId, - installation.id, - ); + return build(githubRepositories, githubRepositories.installationId, installationId); }apps/api/src/app/api/integrations/github/install/route.ts (2)
15-23: Missing UUID format validation fororganizationId.The
organizationIdis used directly in a database query without validating it's a valid UUID. While Drizzle will likely fail gracefully, adding validation provides better error messages and prevents unnecessary DB calls.Proposed fix
+import { z } from "zod"; + +const uuidSchema = z.string().uuid(); + export async function GET(request: Request) { const session = await auth.api.getSession({ headers: request.headers }); if (!session?.user) { return Response.json({ error: "Unauthorized" }, { status: 401 }); } const url = new URL(request.url); const organizationId = url.searchParams.get("organizationId"); if (!organizationId) { return Response.json( { error: "Missing organizationId parameter" }, { status: 400 }, ); } + + if (!uuidSchema.safeParse(organizationId).success) { + return Response.json( + { error: "Invalid organizationId format" }, + { status: 400 }, + ); + }
50-52: Hardcoded GitHub App name reduces flexibility.The app name
superset-appis hardcoded. Consider extracting to an environment variable for different environments (staging, production) or deriving it from the app configuration.Proposed fix
+// In env.ts, add: +// GITHUB_APP_NAME: z.string().min(1), - const installUrl = new URL( - `https://github.com/apps/superset-app/installations/new`, - ); + const installUrl = new URL( + `https://github.com/apps/${env.GITHUB_APP_NAME}/installations/new`, + );apps/api/src/app/api/integrations/github/callback/route.ts (1)
11-14: State schema should validate UUIDs, not just non-empty strings.The
organizationIdanduserIdshould be validated as UUIDs to ensure they match the expected format from the install route and prevent potential issues downstream.Proposed fix
const stateSchema = z.object({ - organizationId: z.string().min(1), - userId: z.string().min(1), + organizationId: z.string().uuid(), + userId: z.string().uuid(), });plans/github-integration-review.md (1)
146-152: Add language specifiers to fenced code blocks for better rendering.The code blocks on lines 146 and 151 are missing language specifiers, which affects syntax highlighting and readability.
Proposed fix
-``` +```text usePRStatus hook → trpc.workspaces.getGitHubStatus → fetchGitHubPRStatus → gh CLITarget flow:
-+text
usePRStatus hook → Electric SQL query → githubPullRequests collectionapps/api/src/app/api/integrations/github/webhook/route.ts (1)
56-59: Signature error detection via string matching is fragile.Checking
error.message.includes("signature")to determine if it's an authentication error is brittle and could break if Octokit changes error messages. Consider checking the error type or code if available.Proposed fix
+ // Check for signature verification failures + const isSignatureError = + error instanceof Error && + (error.message.includes("signature") || + error.name === "SignatureVerificationError"); + - const status = - error instanceof Error && error.message.includes("signature") ? 401 : 500; + const status = isSignatureError ? 401 : 500;apps/api/src/app/api/integrations/github/webhook/webhooks.ts (2)
124-127: Document that PR stats may be unavailable in webhook payloads.The
additions,deletions, andchangedFilesfields are defaulting to 0, which is correct since these fields aren't always present in webhook payloads (only in direct API calls). Consider adding a comment for clarity.- additions: pr.additions ?? 0, - deletions: pr.deletions ?? 0, - changedFiles: pr.changed_files ?? 0, + additions: pr.additions ?? 0, // May be 0 in webhooks; populated by initial sync + deletions: pr.deletions ?? 0, + changedFiles: pr.changed_files ?? 0,
261-283: Consider defensive handling for malformed checks data.The checks array is cast from
jsonband could potentially contain unexpected data. While the fallback handles null, malformed entries could cause issues. Current implementation is acceptable for internal data but could benefit from validation.apps/api/src/app/api/integrations/github/jobs/initial-sync/route.ts (4)
52-52: UnusedorganizationIdparameter.The
organizationIdis validated and destructured but never used in the sync logic. Either remove it from the schema or document why it's included (perhaps for future use or logging).
78-101: Sequential database inserts could be batched.Upserting repositories one-by-one in a loop creates N database round-trips. For large installations with many repositories, this can be slow. Consider batching the upserts into a single transaction or using Drizzle's batch insert capabilities.
Proposed improvement using Promise.all for parallel execution
-// Upsert repositories -for (const repo of repos) { - await db - .insert(githubRepositories) - .values({ - installationId: installationDbId, - repoId: String(repo.id), - owner: repo.owner.login, - name: repo.name, - fullName: repo.full_name, - defaultBranch: repo.default_branch ?? "main", - isPrivate: repo.private, - }) - .onConflictDoUpdate({ - target: [githubRepositories.repoId], - set: { - owner: repo.owner.login, - name: repo.name, - fullName: repo.full_name, - defaultBranch: repo.default_branch ?? "main", - isPrivate: repo.private, - updatedAt: new Date(), - }, - }); -} +// Upsert repositories in parallel +await Promise.all( + repos.map((repo) => + db + .insert(githubRepositories) + .values({ + installationId: installationDbId, + repoId: String(repo.id), + owner: repo.owner.login, + name: repo.name, + fullName: repo.full_name, + defaultBranch: repo.default_branch ?? "main", + isPrivate: repo.private, + }) + .onConflictDoUpdate({ + target: [githubRepositories.repoId], + set: { + owner: repo.owner.login, + name: repo.name, + fullName: repo.full_name, + defaultBranch: repo.default_branch ?? "main", + isPrivate: repo.private, + updatedAt: new Date(), + }, + }) + ) +);
132-156: Extract check type and status computation to named constants/helpers.The inline type annotations are verbose and repeated. Per coding guidelines, extract magic strings to named constants and consider a helper function for status computation.
Proposed refactor
// At module top const CHECK_CONCLUSIONS = { FAILURE: "failure", TIMED_OUT: "timed_out", } as const; const CHECKS_STATUS = { NONE: "none", SUCCESS: "success", PENDING: "pending", FAILURE: "failure", } as const; type CheckResult = { name: string; status: string; conclusion: string | null; detailsUrl?: string; }; function computeChecksStatus(checks: CheckResult[]): string { if (checks.length === 0) return CHECKS_STATUS.NONE; const hasFailure = checks.some( (c) => c.conclusion === CHECK_CONCLUSIONS.FAILURE || c.conclusion === CHECK_CONCLUSIONS.TIMED_OUT ); const hasPending = checks.some((c) => c.status !== "completed"); if (hasFailure) return CHECKS_STATUS.FAILURE; if (hasPending) return CHECKS_STATUS.PENDING; return CHECKS_STATUS.SUCCESS; }
105-111: Unnecessary database query inside loop.The code fetches the repository from the database inside the PR loop, but it was just inserted in the previous loop. Consider building a map of
repoId -> dbRepoduring the upsert phase to avoid N additional queries.Proposed optimization
// After upserting repos, fetch all in one query const dbRepos = await db .select() .from(githubRepositories) .where(eq(githubRepositories.installationId, installationDbId)); const repoMap = new Map(dbRepos.map((r) => [r.repoId, r])); // Then in the PR loop: for (const repo of repos) { const dbRepo = repoMap.get(String(repo.id)); if (!dbRepo) continue; // ... rest of PR processing }packages/db/src/schema/github.ts (3)
48-54: Redundant index oninstallationId.Line 50-52 creates an index on
installationId, but theunique()constraint on line 28 already creates an implicit unique index. The explicit index at line 50-52 is redundant.Similarly, line 53 creates an index on
organizationId, but the unique constraint at line 49 already provides one.Proposed fix - remove redundant indexes
(table) => [ unique("github_installations_org_unique").on(table.organizationId), - index("github_installations_installation_id_idx").on( - table.installationId, - ), - index("github_installations_org_idx").on(table.organizationId), ],
27-30: Consider using an enum foraccountType.The
accountTypecolumn has a fixed set of values ("Organization" | "User"). Using a text column allows invalid values. Consider using a PostgreSQL enum or adding a check constraint for type safety.
160-170: Consider adding an index onauthorLogin.If you plan to filter PRs by author (a common use case), adding an index on
authorLoginwould improve query performance. The current indexes cover repository, branch, state, and status filtering but not author-based queries.Proposed addition
(table) => [ unique("github_pull_requests_repo_pr_unique").on( table.repositoryId, table.prNumber, ), index("github_pull_requests_repo_idx").on(table.repositoryId), index("github_pull_requests_head_branch_idx").on(table.headBranch), index("github_pull_requests_state_idx").on(table.state), index("github_pull_requests_checks_status_idx").on(table.checksStatus), index("github_pull_requests_synced_at_idx").on(table.lastSyncedAt), + index("github_pull_requests_author_idx").on(table.authorLogin), ],packages/db/drizzle/0008_add_github_integration_tables.sql (1)
68-69: Redundant indexes in migration.Lines 68-69 create indexes on
installation_idandorganization_idforgithub_installations, but unique constraints on lines 15-16 already create implicit indexes. This mirrors the redundancy in the schema definition.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
apps/api/package.jsonapps/api/src/app/api/electric/[...path]/utils.tsapps/api/src/app/api/integrations/github/callback/route.tsapps/api/src/app/api/integrations/github/install/route.tsapps/api/src/app/api/integrations/github/jobs/initial-sync/route.tsapps/api/src/app/api/integrations/github/octokit.tsapps/api/src/app/api/integrations/github/webhook/route.tsapps/api/src/app/api/integrations/github/webhook/webhooks.tsapps/api/src/env.tsapps/desktop/src/renderer/contexts/CollectionsProvider/collections.tspackages/db/drizzle/0008_add_github_integration_tables.sqlpackages/db/drizzle/meta/0008_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/enums.tspackages/db/src/schema/github.tspackages/db/src/schema/index.tsplans/github-integration-review.md
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/api/src/env.tsapps/api/src/app/api/integrations/github/callback/route.tspackages/db/src/schema/enums.tsapps/api/src/app/api/integrations/github/webhook/route.tsapps/api/src/app/api/integrations/github/install/route.tsapps/api/src/app/api/integrations/github/jobs/initial-sync/route.tsapps/desktop/src/renderer/contexts/CollectionsProvider/collections.tsapps/api/src/app/api/integrations/github/octokit.tsapps/api/src/app/api/electric/[...path]/utils.tspackages/db/src/schema/index.tspackages/db/src/schema/github.tsapps/api/src/app/api/integrations/github/webhook/webhooks.ts
apps/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/api/**/*.{ts,tsx}: Use TRPCError with appropriate error codes: NOT_FOUND, UNAUTHORIZED, FORBIDDEN, BAD_REQUEST, INTERNAL_SERVER_ERROR, NOT_IMPLEMENTED
tRPC procedures and API route handlers should validate and delegate; keep orchestrators thin
Extract business logic from tRPC procedures into utility functions when logic exceeds ~50 lines, is used by multiple procedures, or needs independent testing
Validate at boundaries using Zod schemas for tRPC inputs and API route bodies
Files:
apps/api/src/env.tsapps/api/src/app/api/integrations/github/callback/route.tsapps/api/src/app/api/integrations/github/webhook/route.tsapps/api/src/app/api/integrations/github/install/route.tsapps/api/src/app/api/integrations/github/jobs/initial-sync/route.tsapps/api/src/app/api/integrations/github/octokit.tsapps/api/src/app/api/electric/[...path]/utils.tsapps/api/src/app/api/integrations/github/webhook/webhooks.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/api/src/env.tsapps/api/src/app/api/integrations/github/callback/route.tsapps/api/src/app/api/integrations/github/webhook/route.tsapps/api/src/app/api/integrations/github/install/route.tsapps/api/src/app/api/integrations/github/jobs/initial-sync/route.tsapps/desktop/src/renderer/contexts/CollectionsProvider/collections.tsapps/api/src/app/api/integrations/github/octokit.tsapps/api/src/app/api/electric/[...path]/utils.tsapps/api/src/app/api/integrations/github/webhook/webhooks.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/api/src/env.tsapps/api/src/app/api/integrations/github/callback/route.tspackages/db/src/schema/enums.tsapps/api/src/app/api/integrations/github/webhook/route.tsapps/api/src/app/api/integrations/github/install/route.tsapps/api/src/app/api/integrations/github/jobs/initial-sync/route.tsapps/desktop/src/renderer/contexts/CollectionsProvider/collections.tsapps/api/src/app/api/integrations/github/octokit.tsapps/api/src/app/api/electric/[...path]/utils.tspackages/db/src/schema/index.tspackages/db/src/schema/github.tsapps/api/src/app/api/integrations/github/webhook/webhooks.ts
packages/db/src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Schema definitions must be in packages/db/src/ using Drizzle ORM
Files:
packages/db/src/schema/enums.tspackages/db/src/schema/index.tspackages/db/src/schema/github.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/renderer/contexts/CollectionsProvider/collections.ts
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Files:
apps/desktop/src/renderer/contexts/CollectionsProvider/collections.ts
🧠 Learnings (3)
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to **/*.{ts,tsx} : Extract magic numbers and hardcoded values to named constants at module top
Applied to files:
packages/db/src/schema/enums.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to packages/db/src/**/*.ts : Schema definitions must be in packages/db/src/ using Drizzle ORM
Applied to files:
packages/db/src/schema/index.tspackages/db/src/schema/github.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/**/*.{ts,tsx} : Use Drizzle ORM for all database operations - never use raw SQL
Applied to files:
packages/db/src/schema/github.ts
🧬 Code graph analysis (7)
apps/api/src/app/api/integrations/github/callback/route.ts (3)
apps/api/src/app/api/integrations/github/install/route.ts (1)
GET(8-56)apps/api/src/app/api/integrations/github/octokit.ts (1)
githubApp(6-11)packages/db/src/schema/github.ts (1)
githubInstallations(16-55)
apps/api/src/app/api/integrations/github/webhook/route.ts (2)
packages/db/src/schema/ingest.ts (1)
webhookEvents(15-47)apps/api/src/app/api/integrations/github/webhook/webhooks.ts (1)
webhooks(13-13)
apps/api/src/app/api/integrations/github/install/route.ts (3)
apps/api/src/app/api/integrations/github/callback/route.ts (1)
GET(20-137)packages/db/src/schema/auth.ts (1)
members(105-122)apps/api/src/env.ts (1)
env(4-51)
apps/desktop/src/renderer/contexts/CollectionsProvider/collections.ts (1)
packages/db/src/schema/github.ts (4)
githubRepositories(61-92)SelectGithubRepository(95-95)githubPullRequests(98-171)SelectGithubPullRequest(174-174)
apps/api/src/app/api/integrations/github/octokit.ts (1)
apps/api/src/env.ts (1)
env(4-51)
apps/api/src/app/api/electric/[...path]/utils.ts (1)
packages/db/src/schema/github.ts (3)
githubInstallations(16-55)githubRepositories(61-92)githubPullRequests(98-171)
apps/api/src/app/api/integrations/github/webhook/webhooks.ts (2)
apps/api/src/env.ts (1)
env(4-51)packages/db/src/schema/github.ts (3)
githubInstallations(16-55)githubRepositories(61-92)githubPullRequests(98-171)
🪛 LanguageTool
plans/github-integration-review.md
[uncategorized] ~50-~50: The official name of this software platform is spelled with a capital “H”.
Context: ...es** (Priority: CRITICAL) Problem: github_repositories and `github_pull_requests...
(GITHUB)
[uncategorized] ~50-~50: The official name of this software platform is spelled with a capital “H”.
Context: ... Problem: github_repositories and github_pull_requests tables won't sync to des...
(GITHUB)
🪛 markdownlint-cli2 (0.18.1)
plans/github-integration-review.md
146-146: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
151-151: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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 (19)
apps/api/src/app/api/electric/[...path]/utils.ts (1)
53-69: LGTM! Correct filtering logic for GitHub repositories.The installation lookup and filter by
installationIdcorrectly scopes repositories to the organization's GitHub installation.packages/db/src/schema/index.ts (1)
2-2: LGTM!The GitHub schema export follows the established pattern and correctly exposes the new GitHub integration tables through the package's public API. Based on learnings, schema definitions are correctly placed in
packages/db/src/.packages/db/drizzle/meta/_journal.json (1)
60-67: LGTM!The migration journal entry follows the established format with correct sequential indexing and a descriptive tag name.
apps/api/src/app/api/integrations/github/octokit.ts (1)
1-11: LGTM! Clean GitHub App configuration.The configuration is minimal and correctly uses environment variables for credentials. The module-level initialization is appropriate since
env.tsvalidates all required variables at startup.apps/api/src/env.ts (1)
24-26: LGTM! Server-only GitHub credentials properly validated.The environment variables are correctly placed in the
serversection and follow the existing pattern used for Linear integration credentials.packages/db/src/schema/enums.ts (1)
26-26: LGTM!The addition of
"github"to the integration provider values correctly expands the enum. The derived Zod schema and TypeScript type will automatically include the new value. The corresponding database migration (0008_add_github_integration_tables.sql) properly updates the PostgreSQLintegration_providerenum type with the new value.apps/api/src/app/api/integrations/github/callback/route.ts (1)
80-109: LGTM - Installation upsert handles reinstallation correctly.Good use of
onConflictDoUpdateto handle reinstallation scenarios, clearing suspension state and updating all relevant fields.apps/api/src/app/api/integrations/github/webhook/route.ts (1)
28-43: LGTM - Webhook verification and status tracking pattern is solid.Good approach of storing the event first, then verifying and processing, with proper status updates on success.
apps/desktop/src/renderer/contexts/CollectionsProvider/collections.ts (2)
144-158: LGTM - Read-only collections follow established patterns.The new GitHub collections correctly follow the existing pattern for read-only data synced via Electric SQL, with proper org-scoping and column mapping.
144-174: No action needed—Electric SQL proxy is already configured for these tables.The
github_repositoriesandgithub_pull_requeststables are already registered inAllowedTable(lines 19–20) and have correspondingbuildWhereClauseimplementations (lines 53–105) with proper organization-scoped filtering via GitHub installations.Likely an incorrect or invalid review comment.
packages/db/drizzle/meta/0008_snapshot.json (2)
679-842: LGTM - GitHub installations schema is properly defined.The schema snapshot correctly captures the
github_installationstable with appropriate foreign keys toauth.organizationsandauth.users, unique constraints oninstallation_idandorganization_id, and useful indexes.
1117-1253: LGTM - GitHub repositories schema with proper cascade relationships.The
github_repositoriestable correctly referencesgithub_installationswith cascade delete, and includes indexes oninstallation_idandfull_namefor efficient lookups.apps/api/src/app/api/integrations/github/webhook/webhooks.ts (3)
15-37: LGTM - Installation lifecycle handlers are well-structured.Good use of consistent logging with
[github/webhook]prefix and proper handling of suspend/unsuspend states.
186-228: Review decision logic intentionally skips "commented" reviews.The handler correctly maps only
approvedandchanges_requestedstates, returning early for other review types (like comments). This is the expected behavior since comments don't affect the review decision.
52-68: Usedefault_branchfrom payload instead of hardcoding "main".The webhook payload for repository events includes the default branch. Hardcoding "main" will be incorrect for repositories using "master" or custom default branches.
Proposed fix
for (const repo of payload.repositories_added) { const [owner, name] = repo.full_name.split("/"); console.log("[github/webhook] Repository added:", repo.full_name); await db .insert(githubRepositories) .values({ installationId: installation.id, repoId: String(repo.id), owner: owner ?? "", name: name ?? repo.name, fullName: repo.full_name, - defaultBranch: "main", + defaultBranch: "main", // Note: installation_repositories event doesn't include default_branch isPrivate: repo.private, }) .onConflictDoNothing(); }Note: The
installation_repositories.addedevent payload doesn't includedefault_branch. Consider fetching it via API during initial sync, or leave the comment to document this limitation.Likely an incorrect or invalid review comment.
packages/db/src/schema/github.ts (1)
16-55: Well-structured schema with appropriate relationships.The schema correctly uses Drizzle ORM per project guidelines, establishes proper FK relationships with cascade deletes, and includes comprehensive indexing for expected query patterns. The type exports using
$inferInsertand$inferSelectfollow best practices.packages/db/drizzle/0008_add_github_integration_tables.sql (2)
2-76: Migration structure looks correct.The migration properly:
- Creates tables in dependency order (installations → repositories → pull_requests)
- Establishes foreign keys with cascade delete semantics
- Creates appropriate indexes for query patterns
- Matches the Drizzle schema definitions
1-1: The concern aboutALTER TYPE ADD VALUEnot being transactional is outdated. Since PostgreSQL 12, enum value additions are fully transactional and will be rolled back if the migration fails. No refactoring or documentation is needed.Likely an incorrect or invalid review comment.
apps/api/src/app/api/integrations/github/jobs/initial-sync/route.ts (1)
104-202: Add rate limit awareness to prevent GitHub API quota exhaustion.The code iterates through repositories, then for each repository fetches all open PRs, and makes an additional API call per PR to fetch checks via
octokit.rest.checks.listForRef(). With multiple repos and PRs, this quickly escalates to many requests. GitHub Apps have a rate limit of 5,000 requests/hour, and there is currently no rate limit checking, backoff strategy, or batch processing with delays.Consider:
- Adding rate limit checking via
octokit.rateLimit.get()before making requests- Implementing exponential backoff or request queuing when approaching limits
- Processing repos/PRs in smaller batches with delays between iterations
Note: While the job is queued via Upstash and has error handling that prevents silent failures, these protections don't prevent quota exhaustion—they only surface it as failures.
| "@octokit/app": "^16.1.2", | ||
| "@octokit/rest": "^22.0.1", | ||
| "@octokit/webhooks": "^14.2.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@octokit/app npm package latest version
💡 Result:
Latest @octokit/app on npm: 16.1.0. [1]
Sources:
- npm package page for @octokit/app. [1]
🌐 Web query:
@octokit/rest npm package latest version
💡 Result:
The latest published version of @octokit/rest is 22.0.0. [1][2]
Sources:
- npm package page for @octokit/rest. [1]
- Yarn package listing (shows 22.0.0 published May 25, 2025). [2]
🌐 Web query:
@octokit/webhooks npm package latest version
💡 Result:
Latest published version (npm) of @octokit/webhooks: 13.8.1. [1]
Sources:
[1] npm package page for @octokit/webhooks.
Update Octokit dependencies to use published versions.
The specified versions do not exist on npm:
@octokit/app@^16.1.2— latest published is 16.1.0@octokit/rest@^22.0.1— latest published is 22.0.0@octokit/webhooks@^14.2.0— latest published is 13.8.1
Update to versions that are actually available on npm to ensure the dependencies can be installed.
🤖 Prompt for AI Agents
In @apps/api/package.json around lines 16 - 18, The package.json lists
non-existent Octokit versions; update the dependency entries for "@octokit/app",
"@octokit/rest", and "@octokit/webhooks" to the actual published versions (set
"@octokit/app" to "16.1.0", "@octokit/rest" to "22.0.0", and "@octokit/webhooks"
to "13.8.1") and then run your package manager to regenerate the lockfile so
installs succeed.
| const parsed = stateSchema.safeParse( | ||
| JSON.parse(Buffer.from(state, "base64url").toString("utf-8")), | ||
| ); | ||
|
|
||
| if (!parsed.success) { | ||
| return Response.redirect( | ||
| `${env.NEXT_PUBLIC_WEB_URL}/settings/integrations?error=invalid_state`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
JSON.parse can throw on malformed input before Zod validation.
If the state parameter contains invalid base64 or the decoded content is not valid JSON, JSON.parse will throw an unhandled exception before Zod validation runs. This would result in a 500 error instead of a proper redirect.
Proposed fix
- const parsed = stateSchema.safeParse(
- JSON.parse(Buffer.from(state, "base64url").toString("utf-8")),
- );
+ let stateData: unknown;
+ try {
+ stateData = JSON.parse(Buffer.from(state, "base64url").toString("utf-8"));
+ } catch {
+ return Response.redirect(
+ `${env.NEXT_PUBLIC_WEB_URL}/settings/integrations?error=invalid_state`,
+ );
+ }
+
+ const parsed = stateSchema.safeParse(stateData);🤖 Prompt for AI Agents
In @apps/api/src/app/api/integrations/github/callback/route.ts around lines 38 -
46, The code calls JSON.parse on the decoded state which can throw before Zod
validation; update the callback handler around the Buffer.from(...).toString and
JSON.parse so you catch any errors (invalid base64 or invalid JSON) and on error
perform the same redirect used for invalid Zod parsing (redirect to
`${env.NEXT_PUBLIC_WEB_URL}/settings/integrations?error=invalid_state`); then
continue to run stateSchema.safeParse against the parsed object (keep the parsed
variable name) when parsing succeeds.
| if (!installation) { | ||
| return Response.json({ error: "Installation not found", skipped: true }); | ||
| } |
There was a problem hiding this comment.
Missing HTTP status code for "not found" response.
When the installation is not found, the response returns 200 OK by default. This should return a 404 status to properly indicate the resource was not found.
Proposed fix
if (!installation) {
- return Response.json({ error: "Installation not found", skipped: true });
+ return Response.json({ error: "Installation not found", skipped: true }, { status: 404 });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!installation) { | |
| return Response.json({ error: "Installation not found", skipped: true }); | |
| } | |
| if (!installation) { | |
| return Response.json({ error: "Installation not found", skipped: true }, { status: 404 }); | |
| } |
🤖 Prompt for AI Agents
In @apps/api/src/app/api/integrations/github/jobs/initial-sync/route.ts around
lines 60 - 62, The "installation not found" branch in the initial-sync route
returns a 200 OK by default; update the Response.json call in the installation
missing branch (the if (!installation) check in route.ts) to return a 404 status
by passing an explicit status option so the response correctly signals "Not
Found" instead of OK.
| eventId: deliveryId ?? `github-${Date.now()}`, | ||
| eventType: eventType ?? "unknown", | ||
| payload: JSON.parse(body), | ||
| status: "pending", | ||
| }) | ||
| .returning(); |
There was a problem hiding this comment.
JSON.parse can throw on malformed webhook payload.
If GitHub sends a malformed payload or the body is empty, JSON.parse(body) will throw an unhandled exception. This should be wrapped in a try-catch to return a proper error response.
Proposed fix
+ let payload: unknown;
+ try {
+ payload = JSON.parse(body);
+ } catch {
+ console.error("[github/webhook] Invalid JSON payload");
+ return Response.json({ error: "Invalid JSON payload" }, { status: 400 });
+ }
+
const [webhookEvent] = await db
.insert(webhookEvents)
.values({
provider: "github",
eventId: deliveryId ?? `github-${Date.now()}`,
eventType: eventType ?? "unknown",
- payload: JSON.parse(body),
+ payload,
status: "pending",
})
.returning();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| eventId: deliveryId ?? `github-${Date.now()}`, | |
| eventType: eventType ?? "unknown", | |
| payload: JSON.parse(body), | |
| status: "pending", | |
| }) | |
| .returning(); | |
| let payload: unknown; | |
| try { | |
| payload = JSON.parse(body); | |
| } catch { | |
| console.error("[github/webhook] Invalid JSON payload"); | |
| return Response.json({ error: "Invalid JSON payload" }, { status: 400 }); | |
| } | |
| const [webhookEvent] = await db | |
| .insert(webhookEvents) | |
| .values({ | |
| provider: "github", | |
| eventId: deliveryId ?? `github-${Date.now()}`, | |
| eventType: eventType ?? "unknown", | |
| payload, | |
| status: "pending", | |
| }) | |
| .returning(); |
🤖 Prompt for AI Agents
In @apps/api/src/app/api/integrations/github/webhook/route.ts around lines 17 -
22, The code calls JSON.parse(body) directly when constructing the webhook event
(eventId/eventType/deliveryId block) which will throw on empty or malformed
bodies; wrap the parse in a try-catch (e.g., const parsed = safeParse(body); or
try { parsed = JSON.parse(body) } catch (err) { log the error and return a
400/422 HTTP response with a clear message instead of proceeding }) and use the
parsed variable when inserting the record (payload: parsed) so malformed
payloads are handled gracefully and do not cause an unhandled exception.
Decided to abandon this integration as we don't really have a product vision for it yet (was going to be for getting the current state of pull requests for a team, but it honestly is a degraded experience for hobbyists compared to the
ghcli polling approach currently implemented (you'd need to install our github app in the repo, can only tie the repo to one).Creating a PR in case we want this for another feature (i.e. our agents feature)