feat: GitHub App webhook handler for repo installation#67
Merged
Conversation
- Webhook route at /api/webhooks/github with HMAC SHA-256 signature verification - Handles installation (created/deleted) and installation_repositories (added/removed) events - Zod validation on all payloads with int/positive constraints on GitHub IDs - BigInt conversion for GitHub IDs matching schema's bigint column type - onConflictDoUpdate for re-installations (refreshes installationId) - DB singleton (lib/db.ts) for shared Drizzle instance across route handlers - Re-exported drizzle-orm operators from @codeindexer/db to avoid pnpm dual-instance issue - 17 unit tests covering signature verification, Zod schemas, and BigInt handling
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughImplements GitHub App webhook handler for repository installation events with HMAC signature verification, Zod payload validation, and database repository upsertion. Includes comprehensive test coverage, new database initialization module, and drizzle-orm operator exports. Changes
Sequence DiagramsequenceDiagram
participant Client as GitHub App
participant Handler as Webhook Handler
participant Crypto as Crypto Verify
participant Zod as Zod Validator
participant DB as Database
Client->>Handler: POST /api/webhooks/github<br/>(X-Hub-Signature-256, X-GitHub-Event)
Handler->>Crypto: Verify HMAC signature
alt Signature Invalid
Crypto-->>Handler: Verification failed
Handler-->>Client: 401 {error: 'Invalid signature'}
else Signature Valid
Crypto-->>Handler: Verified
Handler->>Zod: Validate JSON payload
alt Validation Fails
Zod-->>Handler: Parse error
Handler-->>Client: 200 {ok: true} (error logged)
else Validation Success
Zod-->>Handler: Valid payload
alt Event Type: installation.created
Handler->>DB: Upsert repositories<br/>(installationId, fullName, etc.)
DB-->>Handler: Rows inserted/updated
else Event Type: installation.deleted
Handler-->>Handler: Log uninstallation
else Event Type: installation_repositories
Handler->>DB: Upsert/delete repos
DB-->>Handler: Complete
end
Handler-->>Client: 200 {ok: true}
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/web/src/app/api/webhooks/github/route.ts (1)
89-111: Consider batching repository upserts for larger installations.Sequential upserts work fine for typical installations with a few repositories, but could become slow for organizations with many repos. Drizzle supports batch inserts, though combining with per-row
onConflictDoUpdatevalues requires a transaction approach.💡 Batched approach using transaction
async function upsertRepos( userId: string, installationId: number, repositories: z.infer<typeof repoSchema>[], ) { await db.transaction(async (tx) => { await Promise.all( repositories.map((repo) => tx .insert(repos) .values({ userId, githubId: BigInt(repo.id), fullName: repo.full_name, defaultBranch: repo.default_branch, installationId: BigInt(installationId), isPrivate: repo.private, status: 'pending', }) .onConflictDoUpdate({ target: [repos.userId, repos.githubId], set: { installationId: BigInt(installationId), defaultBranch: repo.default_branch, isPrivate: repo.private, status: 'pending', updatedAt: new Date(), }, }) ) ); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/webhooks/github/route.ts` around lines 89 - 111, Batch the per-repository upserts into a single transaction to improve performance for large installations: create a helper like upsertRepos that accepts userId, installationId, and repositories and inside call db.transaction(async (tx) => Promise.all(repositories.map(repo => tx.insert(repos).values({ ... }).onConflictDoUpdate({ target: [repos.userId, repos.githubId], set: { ... } })))) so all inserts/upserts run in parallel within one transaction rather than sequential awaits; reference the repos table, onConflictDoUpdate, db.transaction and the current per-repo insert block to replace the for-loop with this transactional batch helper.apps/web/src/app/api/webhooks/github/webhook.test.ts (1)
5-41: Consider extracting shared schemas to reduce duplication.The schemas and
verifySignaturefunction are duplicated between the test file androute.ts. While the comment acknowledges this, drifting implementations could cause false test passes if the production code changes but tests validate against stale schemas.Consider extracting these to a shared module (e.g.,
webhook-schemas.ts) that both the route and tests import.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/webhooks/github/webhook.test.ts` around lines 5 - 41, The test duplicates the webhook schemas and verifySignature implementation (repoSchema, installationSchema, installationEventSchema, installationReposEventSchema, and verifySignature) which can drift from the route handler; extract these shared definitions into a single module (e.g., webhook-schemas or webhook-utils) and update both the route handler and apps/web test to import the schemas and verifySignature from that module so production and tests use the same logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/app/api/webhooks/github/route.ts`:
- Line 76: Replace direct console calls in the GitHub webhook handler with a
Pino logger: add an import pino and create const logger = pino({ name:
'github-webhook' }), then change console.error(`Webhook ${event} processing
failed:`, error) to logger.error({ event, err: error }, 'Webhook processing
failed') and similarly replace console.log(...) → logger.info(...) and
console.warn(...) → logger.warn(...) for other occurrences (refer to the webhook
route handler and the spots around the existing console usage at the event/error
handling blocks and the lines noted: 124, 132-134, 142, 163) so all logging uses
the logger instance and structured log fields instead of console.
- Around line 153-156: When repositories_added is present but
findUserByGitHubAccountId(String(installation.account.id)) returns no user, add
a warning log (using the same Pino logger used elsewhere, not console.warn)
before returning to improve observability; update the handler in route.ts (the
block that currently checks repositories_added and calls
findUserByGitHubAccountId) to log a clear warning including the githubAccountId
and installation.id, similar to how handleInstallation logs, then return.
---
Nitpick comments:
In `@apps/web/src/app/api/webhooks/github/route.ts`:
- Around line 89-111: Batch the per-repository upserts into a single transaction
to improve performance for large installations: create a helper like upsertRepos
that accepts userId, installationId, and repositories and inside call
db.transaction(async (tx) => Promise.all(repositories.map(repo =>
tx.insert(repos).values({ ... }).onConflictDoUpdate({ target: [repos.userId,
repos.githubId], set: { ... } })))) so all inserts/upserts run in parallel
within one transaction rather than sequential awaits; reference the repos table,
onConflictDoUpdate, db.transaction and the current per-repo insert block to
replace the for-loop with this transactional batch helper.
In `@apps/web/src/app/api/webhooks/github/webhook.test.ts`:
- Around line 5-41: The test duplicates the webhook schemas and verifySignature
implementation (repoSchema, installationSchema, installationEventSchema,
installationReposEventSchema, and verifySignature) which can drift from the
route handler; extract these shared definitions into a single module (e.g.,
webhook-schemas or webhook-utils) and update both the route handler and apps/web
test to import the schemas and verifySignature from that module so production
and tests use the same logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48cc6635-4aea-43c1-a23b-0aa3a1a714ec
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
apps/web/package.jsonapps/web/src/app/api/webhooks/github/route.tsapps/web/src/app/api/webhooks/github/webhook.test.tsapps/web/src/lib/db.tspackages/db/src/index.ts
| break; | ||
| } | ||
| } catch (error) { | ||
| console.error(`Webhook ${event} processing failed:`, error); |
There was a problem hiding this comment.
Replace console.log/console.error with Pino logger.
The coding guidelines specify using Pino for logging throughout the application. Multiple locations use console.log and console.error instead.
🔧 Suggested approach
Import and use Pino logger:
import pino from 'pino';
const logger = pino({ name: 'github-webhook' });
// Then replace:
// console.error(...) → logger.error(...)
// console.log(...) → logger.info(...)
// console.warn(...) → logger.warn(...)As per coding guidelines: "Use Pino for logging throughout the application".
Also applies to: 124-124, 132-134, 142-142, 163-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/api/webhooks/github/route.ts` at line 76, Replace direct
console calls in the GitHub webhook handler with a Pino logger: add an import
pino and create const logger = pino({ name: 'github-webhook' }), then change
console.error(`Webhook ${event} processing failed:`, error) to logger.error({
event, err: error }, 'Webhook processing failed') and similarly replace
console.log(...) → logger.info(...) and console.warn(...) → logger.warn(...) for
other occurrences (refer to the webhook route handler and the spots around the
existing console usage at the event/error handling blocks and the lines noted:
124, 132-134, 142, 163) so all logging uses the logger instance and structured
log fields instead of console.
Comment on lines
+153
to
+156
| if (repositories_added?.length) { | ||
| const githubAccountId = String(installation.account.id); | ||
| const userAccount = await findUserByGitHubAccountId(githubAccountId); | ||
| if (!userAccount) return; |
There was a problem hiding this comment.
Add logging when user lookup fails.
Unlike handleInstallation (line 142), this handler silently returns when no user is found. For observability, log a warning to help debug dropped webhooks.
🔧 Proposed fix
const userAccount = await findUserByGitHubAccountId(githubAccountId);
- if (!userAccount) return;
+ if (!userAccount) {
+ console.warn(`No user found for GitHub account ${githubAccountId} — repositories_added dropped`);
+ return;
+ }(And replace console.warn with Pino logger per the earlier comment.)
📝 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.
Suggested change
| if (repositories_added?.length) { | |
| const githubAccountId = String(installation.account.id); | |
| const userAccount = await findUserByGitHubAccountId(githubAccountId); | |
| if (!userAccount) return; | |
| if (repositories_added?.length) { | |
| const githubAccountId = String(installation.account.id); | |
| const userAccount = await findUserByGitHubAccountId(githubAccountId); | |
| if (!userAccount) { | |
| logger.warn(`No user found for GitHub account ${githubAccountId} — repositories_added dropped`); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/api/webhooks/github/route.ts` around lines 153 - 156, When
repositories_added is present but
findUserByGitHubAccountId(String(installation.account.id)) returns no user, add
a warning log (using the same Pino logger used elsewhere, not console.warn)
before returning to improve observability; update the handler in route.ts (the
block that currently checks repositories_added and calls
findUserByGitHubAccountId) to log a clear warning including the githubAccountId
and installation.id, similar to how handleInstallation logs, then return.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/api/webhooks/githubwith HMAC SHA-256 signature verification (raw 32-byte digest comparison for constant-time safety)installation(created/deleted) andinstallation_repositories(added/removed) events.int().positive()constraints matching bigint schema columnsonConflictDoUpdatefor re-installations — refreshesinstallationIdinstead of silently ignoringlib/db.ts) for shared Drizzle instance across route handlersdrizzle-ormoperators from@codeindexer/dbto avoid pnpm dual-instance type conflictsFiles
apps/web/src/app/api/webhooks/github/route.tsapps/web/src/app/api/webhooks/github/webhook.test.tsapps/web/src/lib/db.tspackages/db/src/index.tseq,and,or,desc,asc,sqlfrom drizzle-ormTest plan
pnpm format:checkpassespnpm typecheckpasses (web + db)pnpm lintpassesCloses #59
Summary by CodeRabbit
New Features
Tests
Chores