feat(webapp): github app installation flow #2463
Conversation
|
WalkthroughAdds GitHub App support: environment schema now includes a discriminated union gated by GITHUB_APP_ENABLED. Introduces GitHub App services (initialization, link/update installation, repo pagination), a session-based install flow (create/validate/destroy cookie session), two new Remix routes for install and callback handling, utility rename/strengthening (safeRedirect → sanitizeRedirectPath), Prisma schema/models and SQL migration for GithubAppInstallation and GithubRepository, an octokit dependency, and a small import/migration comment tweak. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Reasoning: Wide-ranging, heterogeneous changes touching environment validation, security-sensitive session handling, new route control flow, external API integration (Octokit), DB schema + migration, and data persistence logic — each requires careful, separate review. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/webapp/app/routes/auth.github.ts (1)
6-16: Harden redirect and cookie: prevent open redirects; set secure cookie in production.
- Validate redirectTo to avoid open redirects (use remix-utils safeRedirect).
- Mark the cookie as secure in production and set sameSite.
Apply:
@@ -import { type ActionFunction, type LoaderFunction, redirect, createCookie } from "@remix-run/node"; +import { type ActionFunction, type LoaderFunction, redirect, createCookie } from "@remix-run/node"; +import { safeRedirect } from "remix-utils"; +import { env } from "~/env.server"; @@ const url = new URL(request.url); - const redirectTo = url.searchParams.get("redirectTo"); + const redirectTo = safeRedirect(url.searchParams.get("redirectTo") ?? "/", "/"); @@ export const redirectCookie = createCookie("redirect-to", { maxAge: 60 * 60, // 1 hour httpOnly: true, + sameSite: "lax", + secure: env.NODE_ENV === "production", });Also applies to: 29-33
🧹 Nitpick comments (10)
apps/webapp/package.json (1)
160-160: Octokit usage is server‐only; consider scoped packages for GitHub App authOptionally:
- Use
@octokit/app,@octokit/auth-app, and@octokit/rest+@octokit/plugin-paginate-restinstead of the umbrellaoctokitto narrow bundle and typings.- Confirm your
engines.nodemeets Octokit v3.2.1 requirements (Node ≥ 16).apps/webapp/app/env.server.ts (1)
248-250: Nit: comment typo.“refix 250 tokens” → “refill 250 tokens”.
- API_RATE_LIMIT_REFILL_RATE: z.coerce.number().int().default(250), // refix 250 tokens every 10 seconds + API_RATE_LIMIT_REFILL_RATE: z.coerce.number().int().default(250), // refill 250 tokens every 10 secondsapps/webapp/app/routes/_app.github.install/route.tsx (1)
2-2: Consistency nit: use the same redirect import source as the callback route.The callback route uses "@remix-run/node". Consider standardizing import source for redirect across routes.
apps/webapp/app/routes/_app.github.callback/route.tsx (1)
42-44: Redirect to a safe default on invalid session.Optional: consider redirecting to the last known redirect target if available; otherwise "/" is fine.
apps/webapp/app/services/gitHubSession.server.ts (3)
25-27: Also require GITHUB_APP_SLUG when the app feature is enabled.
Defensive check in case env parsing ever regresses.Add a refinement in env.server.ts:
// apps/webapp/app/env.server.ts GITHUB_APP_ENABLED: z.enum(["0","1"]), GITHUB_APP_SLUG: z.string().optional(), }).refine( (e) => e.GITHUB_APP_ENABLED === "0" || !!e.GITHUB_APP_SLUG, { message: "GITHUB_APP_SLUG is required when GITHUB_APP_ENABLED=1" } );
107-112: Consider consuming the session on successful validation.
Prevents replay within the 1h window if the callback is hit twice.You could add a consume helper that validates then returns a Set-Cookie header to clear:
export async function consumeGitHubAppInstallSession(cookieHeader: string | null, state: string) { const result = await validateGitHubAppInstallSession(cookieHeader, state); const destroyHeader = await destroyGitHubAppInstallSession(cookieHeader); return { ...result, destroyHeader }; }
2-2: Use node:crypto import.
Minor Node ESM best practice and avoids bundler shims.-import { randomBytes } from "crypto"; +import { randomBytes } from "node:crypto";internal-packages/database/prisma/schema.prisma (2)
2247-2269: Prefer BigInt for GitHub numeric IDs (future-proof).
GitHub IDs aren’t guaranteed to stay within 32-bit. Using BigInt avoids overflow risk.model GithubAppInstallation { id String @id @default(cuid()) - appInstallationId Int @unique - targetId Int + appInstallationId BigInt @unique + targetId BigInt targetType String accountHandle String permissions Json? repositorySelection GithubRepositorySelection installedBy String? ... }
2271-2289: Indexes and types for repository lookups.
- Use BigInt for githubId.
- Add indexes you’ll likely need (by githubId and fullName).
model GithubRepository { id String @id @default(cuid()) - githubId Int + githubId BigInt name String fullName String htmlUrl String private Boolean defaultBranch String ... - @@unique([installationId, githubId]) - @@index([installationId]) + @@unique([installationId, githubId]) + @@index([installationId]) + @@index([githubId]) + @@index([fullName(ops: raw("text_pattern_ops"))], type: BTree) }internal-packages/database/prisma/migrations/20250829124138_add_gh_installation_schema/migration.sql (1)
5-7: Match SQL types to BigInt if you adopt the Prisma change.
Keeps Prisma and DB in sync and future-proofs IDs.- "appInstallationId" INTEGER NOT NULL, - "targetId" INTEGER NOT NULL, + "appInstallationId" BIGINT NOT NULL, + "targetId" BIGINT NOT NULL, ... - "githubId" INTEGER NOT NULL, + "githubId" BIGINT NOT NULL,Optional helpful indexes:
+CREATE INDEX "GithubRepository_githubId_idx" ON "public"."GithubRepository"("githubId"); +CREATE INDEX "GithubRepository_fullName_idx" ON "public"."GithubRepository" USING btree ("fullName" text_pattern_ops); +CREATE INDEX "GithubAppInstallation_active_idx" ON "public"."GithubAppInstallation"("organizationId") WHERE "deletedAt" IS NULL AND "suspendedAt" IS NULL;Also applies to: 23-24
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
apps/webapp/app/env.server.ts(1 hunks)apps/webapp/app/routes/_app.github.callback/route.tsx(1 hunks)apps/webapp/app/routes/_app.github.install/route.tsx(1 hunks)apps/webapp/app/routes/auth.github.ts(1 hunks)apps/webapp/app/services/gitHub.server.ts(1 hunks)apps/webapp/app/services/gitHubSession.server.ts(1 hunks)apps/webapp/package.json(2 hunks)internal-packages/database/prisma/migrations/20250829124138_add_gh_installation_schema/migration.sql(1 hunks)internal-packages/database/prisma/migrations/migration_lock.toml(1 hunks)internal-packages/database/prisma/schema.prisma(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/app/services/gitHub.server.tsapps/webapp/app/routes/_app.github.callback/route.tsxapps/webapp/app/env.server.tsapps/webapp/app/routes/_app.github.install/route.tsxapps/webapp/app/routes/auth.github.tsapps/webapp/app/services/gitHubSession.server.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/services/gitHub.server.tsapps/webapp/app/routes/_app.github.callback/route.tsxapps/webapp/app/env.server.tsapps/webapp/app/routes/_app.github.install/route.tsxapps/webapp/app/routes/auth.github.tsapps/webapp/app/services/gitHubSession.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/services/gitHub.server.tsapps/webapp/app/routes/_app.github.callback/route.tsxapps/webapp/app/env.server.tsapps/webapp/app/routes/_app.github.install/route.tsxapps/webapp/app/routes/auth.github.tsapps/webapp/app/services/gitHubSession.server.ts
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Files:
apps/webapp/app/services/gitHub.server.tsapps/webapp/app/env.server.tsapps/webapp/app/routes/auth.github.tsapps/webapp/app/services/gitHubSession.server.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead
Files:
apps/webapp/app/services/gitHub.server.tsapps/webapp/app/env.server.tsapps/webapp/app/routes/auth.github.tsapps/webapp/app/services/gitHubSession.server.ts
🧠 Learnings (2)
📚 Learning: 2025-08-29T10:06:49.283Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-08-29T10:06:49.283Z
Learning: Applies to {apps/webapp/**/__tests__/**/*.{ts,tsx},apps/webapp/**/*.{test,spec}.{ts,tsx}} : Do not import app/env.server.ts into tests, either directly or indirectly
Applied to files:
apps/webapp/app/env.server.ts
📚 Learning: 2025-08-29T10:06:49.283Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-08-29T10:06:49.283Z
Learning: Applies to {apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts} : Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Applied to files:
apps/webapp/app/env.server.ts
🧬 Code graph analysis (5)
apps/webapp/app/services/gitHub.server.ts (1)
apps/webapp/app/env.server.ts (1)
env(1157-1157)
apps/webapp/app/routes/_app.github.callback/route.tsx (6)
apps/webapp/app/routes/_app.github.install/route.tsx (1)
loader(15-49)apps/webapp/app/models/message.server.ts (2)
redirectWithErrorMessage(181-198)redirectWithSuccessMessage(162-179)apps/webapp/app/services/gitHubSession.server.ts (1)
validateGitHubAppInstallSession(50-112)apps/webapp/app/services/session.server.ts (1)
requireUser(39-62)apps/webapp/app/db.server.ts (1)
$replica(103-106)apps/webapp/app/services/gitHub.server.ts (1)
linkGitHubAppInstallation(19-56)
apps/webapp/app/env.server.ts (2)
apps/webapp/app/utils/db.ts (1)
isValidDatabaseUrl(1-18)apps/webapp/app/utils/boolEnv.ts (1)
BoolEnv(12-14)
apps/webapp/app/routes/_app.github.install/route.tsx (5)
apps/webapp/app/routes/_app.github.callback/route.tsx (1)
loader(17-95)apps/webapp/app/services/session.server.ts (1)
requireUser(39-62)apps/webapp/app/db.server.ts (1)
$replica(103-106)apps/webapp/app/utils/pathBuilder.ts (1)
newOrganizationPath(96-98)apps/webapp/app/services/gitHubSession.server.ts (1)
createGitHubAppInstallSession(21-45)
apps/webapp/app/services/gitHubSession.server.ts (2)
apps/webapp/app/models/message.server.ts (1)
createCookieSessionStorage(19-28)apps/webapp/app/env.server.ts (1)
env(1157-1157)
🔇 Additional comments (8)
internal-packages/database/prisma/migrations/migration_lock.toml (1)
2-3: LGTM: comment tweak only, no behavioral change.The wording update is fine; provider remains unchanged.
apps/webapp/package.json (1)
57-57: Dependency reordering is harmless.No functional impact from moving @internal/redis below @internal/cache.
apps/webapp/app/routes/auth.github.ts (1)
1-1: Import consolidation looks good.Type-only specifiers with runtime imports from @remix-run/node are fine.
apps/webapp/app/services/gitHub.server.ts (1)
36-56: Use upsert for idempotent installation linkingConfirmed
appInstallationIdis unique; switch toupsertto avoid unique-constraint errors and duplicate repositories.- await prisma.githubAppInstallation.create({ - data: { + await prisma.githubAppInstallation.upsert({ + where: { appInstallationId: installationId }, + create: { appInstallationId: installationId, organizationId, targetId: installation.target_id, targetType: installation.target_type, accountHandle: installation.account ? "login" in installation.account ? installation.account.login : "slug" in installation.account ? installation.account.slug : "-" : "-", permissions: installation.permissions, repositorySelection, repositories: { create: repositories, }, - }, + }, + update: { + organizationId, + targetId: installation.target_id, + targetType: installation.target_type, + accountHandle: installation.account + ? "login" in installation.account + ? installation.account.login + : "slug" in installation.account + ? installation.account.slug + : "-" + : "-", + permissions: installation.permissions, + repositorySelection, + repositories: { + deleteMany: {}, + create: repositories, + }, + }, });apps/webapp/app/services/gitHubSession.server.ts (1)
6-16: Cookie settings look solid (httpOnly, secure in prod, 1h TTL).
Good alignment with our session policy.internal-packages/database/prisma/schema.prisma (3)
154-166: Whitespace-only change.
No action needed.
213-214: Nice: back-reference from Organization to installations.
Keeps navigation cheap for org-scoped queries.
2242-2246: Enum aligns with GitHub’s repository_selection.
Looks good.
myftija
force-pushed
the
gh-app-installation-flow
branch
from
8c5ebd1 to
2809981
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/webapp/app/env.server.ts (1)
6-26: Good fix: preprocess restores safe default for discriminated unionPrepending
GITHUB_APP_ENABLED: "0"viaz.preprocessresolves the prior parse failure when the key is unset. ✔︎apps/webapp/app/services/gitHubSession.server.ts (1)
31-36: Sanitize redirectTo (acknowledging maintainer decision)Storing
redirectTowithout validation can enable open redirects on callback. If you reconsider, gate to safe relative paths before saving.- session.set("redirectTo", redirectTo); + const safeRedirectTo = + typeof redirectTo === "string" && + redirectTo.startsWith("/") && + !redirectTo.startsWith("//") && + !redirectTo.includes("://") + ? redirectTo + : "/app"; + session.set("redirectTo", safeRedirectTo);
🧹 Nitpick comments (7)
internal-packages/database/prisma/schema.prisma (4)
2258-2260: Consider cascading on org delete/update for consistency.
Many models here cascade from Organization; match that behavior unless you intentionally restrict deletes.- organization Organization @relation(fields: [organizationId], references: [id]) + organization Organization @relation(fields: [organizationId], references: [id], onDelete: Cascade, onUpdate: Cascade)
2247-2256: Prefer an enum for targetType.
GitHub installation targets are “User” or “Organization”. An enum prevents drift and eases querying.+enum GithubInstallationTargetType { + USER + ORGANIZATION +} ... - targetType String + targetType GithubInstallationTargetType
2287-2289: Add a lookup index by fullName under an installation (optional).
If you commonly resolve repos by owner/name, this speeds queries; keep githubId as the source of truth.@@unique([installationId, githubId]) - @@index([installationId]) + @@index([installationId]) + @@index([installationId, fullName])
2276-2276: Naming nit: “private” can be confusing in TS contexts.
It’s valid, but consider isPrivate for clarity.- private Boolean + isPrivate Boolean @map("private")Note: Keep the column name stable via @Map.
apps/webapp/app/env.server.ts (2)
9-11: Nit: avoid overriding intentional empty-string valuesUse a nullish check so empty strings don’t get replaced by "0".
- if (!obj || !obj.GITHUB_APP_ENABLED) { + if (obj == null || obj.GITHUB_APP_ENABLED == null) { return { ...obj, GITHUB_APP_ENABLED: "0" }; }
15-24: Optional: validate shapes more strictly when enabledIf helpful, coerce/validate fields tighter (e.g., numeric App ID, PK format).
- GITHUB_APP_ID: z.string(), - GITHUB_APP_PRIVATE_KEY: z.string(), + GITHUB_APP_ID: z.coerce.number().int().positive().transform(String), + GITHUB_APP_PRIVATE_KEY: z.string().refine((v) => v.includes("BEGIN") && v.includes("END"), "Expected a PEM-formatted private key"),apps/webapp/app/services/gitHubSession.server.ts (1)
95-105: Defensive numeric coercion for createdAtSession values are JSON-serialized; coercing guards against unexpected types and future edits.
- const expirationTime = createdAt + 60 * 60 * 1000; + const createdAtMs = Number(createdAt); + if (!Number.isFinite(createdAtMs)) { + logger.warn("GitHub App installation session has non-numeric createdAt", { createdAt }); + return { valid: false, error: "invalid_session_data" }; + } + const expirationTime = createdAtMs + 60 * 60 * 1000; - logger.warn("GitHub App installation session expired", { - createdAt: new Date(createdAt), + logger.warn("GitHub App installation session expired", { + createdAt: new Date(createdAtMs), now: new Date(), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
apps/webapp/app/env.server.ts(1 hunks)apps/webapp/app/routes/_app.github.callback/route.tsx(1 hunks)apps/webapp/app/routes/_app.github.install/route.tsx(1 hunks)apps/webapp/app/routes/auth.github.ts(1 hunks)apps/webapp/app/services/gitHub.server.ts(1 hunks)apps/webapp/app/services/gitHubSession.server.ts(1 hunks)apps/webapp/package.json(1 hunks)internal-packages/database/prisma/migrations/20250829124138_add_gh_installation_schema/migration.sql(1 hunks)internal-packages/database/prisma/migrations/migration_lock.toml(1 hunks)internal-packages/database/prisma/schema.prisma(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal-packages/database/prisma/migrations/migration_lock.toml
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/webapp/app/routes/_app.github.install/route.tsx
- apps/webapp/app/routes/auth.github.ts
- apps/webapp/package.json
- apps/webapp/app/services/gitHub.server.ts
- apps/webapp/app/routes/_app.github.callback/route.tsx
- internal-packages/database/prisma/migrations/20250829124138_add_gh_installation_schema/migration.sql
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/app/services/gitHubSession.server.tsapps/webapp/app/env.server.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/services/gitHubSession.server.tsapps/webapp/app/env.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/services/gitHubSession.server.tsapps/webapp/app/env.server.ts
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Files:
apps/webapp/app/services/gitHubSession.server.tsapps/webapp/app/env.server.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead
Files:
apps/webapp/app/services/gitHubSession.server.tsapps/webapp/app/env.server.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: myftija
PR: triggerdotdev/trigger.dev#2463
File: apps/webapp/app/routes/_app.github.callback/route.tsx:33-44
Timestamp: 2025-09-02T11:27:36.319Z
Learning: In the GitHub App installation callback flow in apps/webapp/app/routes/_app.github.callback/route.tsx, the install session cookie is not cleared after use due to interface limitations with redirectWithSuccessMessage/redirectWithErrorMessage not supporting custom headers. The maintainer accepts this design as the 1-hour cookie expiration provides sufficient protection against replay attacks.
📚 Learning: 2025-09-02T11:27:36.319Z
Learnt from: myftija
PR: triggerdotdev/trigger.dev#2463
File: apps/webapp/app/routes/_app.github.callback/route.tsx:33-44
Timestamp: 2025-09-02T11:27:36.319Z
Learning: In the GitHub App installation callback flow in apps/webapp/app/routes/_app.github.callback/route.tsx, the install session cookie is not cleared after use due to interface limitations with redirectWithSuccessMessage/redirectWithErrorMessage not supporting custom headers. The maintainer accepts this design as the 1-hour cookie expiration provides sufficient protection against replay attacks.
Applied to files:
apps/webapp/app/services/gitHubSession.server.ts
📚 Learning: 2025-09-02T11:18:06.562Z
Learnt from: myftija
PR: triggerdotdev/trigger.dev#2463
File: apps/webapp/app/services/gitHubSession.server.ts:31-36
Timestamp: 2025-09-02T11:18:06.562Z
Learning: In the GitHub App installation flow in apps/webapp/app/services/gitHubSession.server.ts, the redirectTo parameter stored in httpOnly session cookies is considered acceptable without additional validation by the maintainer, as the httpOnly cookie provides sufficient security for this use case.
Applied to files:
apps/webapp/app/services/gitHubSession.server.ts
📚 Learning: 2025-08-29T10:06:49.283Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-08-29T10:06:49.283Z
Learning: Applies to {apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts} : Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Applied to files:
apps/webapp/app/env.server.ts
📚 Learning: 2025-08-29T10:06:49.283Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-08-29T10:06:49.283Z
Learning: Applies to {apps/webapp/**/__tests__/**/*.{ts,tsx},apps/webapp/**/*.{test,spec}.{ts,tsx}} : Do not import app/env.server.ts into tests, either directly or indirectly
Applied to files:
apps/webapp/app/env.server.ts
📚 Learning: 2025-08-14T18:35:44.370Z
Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#2390
File: apps/webapp/app/env.server.ts:764-765
Timestamp: 2025-08-14T18:35:44.370Z
Learning: The BoolEnv helper in apps/webapp/app/utils/boolEnv.ts uses z.preprocess with inconsistent default value types across the codebase - some usages pass boolean defaults (correct) while others pass string defaults (incorrect), leading to type confusion. The helper should enforce boolean-only defaults or have clearer documentation.
Applied to files:
apps/webapp/app/env.server.ts
🧬 Code graph analysis (2)
apps/webapp/app/services/gitHubSession.server.ts (2)
apps/webapp/app/models/message.server.ts (1)
createCookieSessionStorage(19-28)apps/webapp/app/env.server.ts (1)
env(1176-1176)
apps/webapp/app/env.server.ts (3)
apps/webapp/app/utils/db.ts (1)
isValidDatabaseUrl(1-18)apps/webapp/app/v3/services/bulk/BulkActionV2.server.ts (1)
process(87-357)apps/webapp/app/utils/boolEnv.ts (1)
BoolEnv(12-14)
⏰ 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). (10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
🔇 Additional comments (7)
internal-packages/database/prisma/schema.prisma (3)
154-156: Minor relation formatting change looks fine.
No functional impact spotted.
213-214: Organization → GithubAppInstallation inverse relation: LGTM.
Adds needed navigation from orgs to installations.
2242-2246: Enum for repository selection: LGTM.
Simple, future-proof discriminator.apps/webapp/app/env.server.ts (1)
1173-1173: Schema composition looks correctIntersecting with
GithubAppEnvSchemacleanly gates the app settings. No issues.apps/webapp/app/services/gitHubSession.server.ts (3)
6-16: Cookie settings are solid for this flowhttpOnly + 1h max-age + SameSite=Lax + prod-secure is appropriate for the installation round-trip.
21-27: Guard on feature flag is correctEarly exit when the app is disabled prevents accidental entry into the flow.
114-124: Destroy helper is fineAPI returns a Set-Cookie header string; returning empty string on missing header is acceptable.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/webapp/app/routes/_app.github.install/route.tsx (1)
1-9: Gracefully handle disabled GitHub App instead of bubbling 500.Same concern as previously raised; catch the "not enabled" error and show a friendly message.
-import { redirect } from "remix-typedjson"; +import { redirect } from "remix-typedjson"; +import { redirectWithErrorMessage } from "~/models/message.server";- const { url, cookieHeader } = await createGitHubAppInstallSession(org.id, redirect_to); - - return redirect(url, { - headers: { - "Set-Cookie": cookieHeader, - }, - }); + try { + const { url, cookieHeader } = await createGitHubAppInstallSession(org.id, redirectTo); + return redirect(url, { + headers: { "Set-Cookie": cookieHeader }, + }); + } catch (error) { + logger.warn("GitHub App install session could not be created", { error, orgId: org.id }); + return redirectWithErrorMessage(redirectTo, request, "GitHub App is not enabled"); + }Also applies to: 45-51
🧹 Nitpick comments (11)
apps/webapp/app/utils.ts (2)
11-11: Fix JSDoc param wording ("to" → "path").Minor doc mismatch.
- * @param {string} defaultRedirect The redirect to use if the to is unsafe. + * @param {string} defaultRedirect The redirect to use if the path is unsafe.
25-31: Remove redundant absolute-URL parse.Given the earlier startsWith("/") and not startsWith("//") checks,
new URL(path)will always throw; this block never returns and adds overhead.- try { - // should not parse as a full URL - new URL(path); - return defaultRedirect; - } catch {}apps/webapp/app/routes/_app.github.install/route.tsx (2)
11-16: Makeredirect_tooptional and sanitize after parse; tightenorg_slug.Avoid hard-failing when
redirect_tois absent, and ensure non-empty slug.-const QuerySchema = z.object({ - org_slug: z.string(), - redirect_to: z.string().refine((value) => value === sanitizeRedirectPath(value), { - message: "Invalid redirect path", - }), -}); +const QuerySchema = z.object({ + org_slug: z.string().min(1), + redirect_to: z.string().optional(), +});- const { org_slug, redirect_to } = parsed.data; + const { org_slug, redirect_to } = parsed.data; + const redirectTo = sanitizeRedirectPath(redirect_to);- const { url, cookieHeader } = await createGitHubAppInstallSession(org.id, redirect_to); + const { url, cookieHeader } = await createGitHubAppInstallSession(org.id, redirectTo);
22-28: Log structured params, not URLSearchParams instance.Ensures values are captured predictably in logs.
- logger.warn("GitHub App installation redirect with invalid params", { - searchParams, - error: parsed.error, - }); + logger.warn("GitHub App installation redirect with invalid params", { + searchParams: Object.fromEntries(searchParams), + error: parsed.error, + });internal-packages/database/prisma/migrations/20250829124138_add_gh_installation_schema/migration.sql (7)
7-7: Constrain targetType to expected values.Use an enum or CHECK to avoid arbitrary text and data drift (e.g., "User" vs "user").
+-- Constrain installation target type +CREATE TYPE "public"."GithubInstallationTarget" AS ENUM ('ACCOUNT', 'ORGANIZATION'); +ALTER TABLE "public"."GithubAppInstallation" + ALTER COLUMN "targetType" TYPE "public"."GithubInstallationTarget" USING + (CASE UPPER("targetType") WHEN 'ACCOUNT' THEN 'ACCOUNT'::"public"."GithubInstallationTarget" + WHEN 'ORGANIZATION' THEN 'ORGANIZATION'::"public"."GithubInstallationTarget" + ELSE 'ORGANIZATION'::"public"."GithubInstallationTarget" END);
9-9: Normalize permissions JSONB (default and not null).Prevents tri-state semantics and simplifies queries.
+ALTER TABLE "public"."GithubAppInstallation" + ALTER COLUMN "permissions" SET DEFAULT '{}'::jsonb; +UPDATE "public"."GithubAppInstallation" SET "permissions" = '{}'::jsonb WHERE "permissions" IS NULL; +ALTER TABLE "public"."GithubAppInstallation" + ALTER COLUMN "permissions" SET NOT NULL;
38-38: Add partial index for active installations lookups.Likely hot path: "by organization where not deleted/suspended".
+-- Speed up queries for active installations per org +CREATE INDEX "GithubAppInstallation_org_active_idx" + ON "public"."GithubAppInstallation"("organizationId") + WHERE "deletedAt" IS NULL AND "suspendedAt" IS NULL;
21-31: Add lookup indexes for repositories by githubId/fullName.Octokit payloads often use repo ID or slug; dedicated indexes avoid seq scans.
+-- Common lookup patterns +CREATE INDEX "GithubRepository_githubId_idx" ON "public"."GithubRepository"("githubId"); +CREATE INDEX "GithubRepository_fullName_idx" ON "public"."GithubRepository"("fullName");
42-42: Confirm uniqueness scope for repositories across installations.If a repo can’t belong to two installations simultaneously, consider enforcing uniqueness on githubId (optionally partial on active rows). If cross-install duplicates are expected (e.g., transfer history), keep as-is.
Option if needed:
+-- Enforce one active row per GitHub repo id +CREATE UNIQUE INDEX "GithubRepository_githubId_unique_active" + ON "public"."GithubRepository"("githubId") + -- Add "deletedAt" column first if you choose soft-deletes; otherwise omit WHERE. + -- WHERE "deletedAt" IS NULL +;
15-16: updatedAt has no DB default/trigger.Assuming Prisma manages it (@updatedat). If other writers (raw SQL, jobs) touch these tables, consider a DB trigger to avoid nulls/stale values.
+-- Optional: keep updatedAt in sync at the DB layer +CREATE OR REPLACE FUNCTION set_updated_at() RETURNS trigger AS $$ +BEGIN NEW."updatedAt" = CURRENT_TIMESTAMP; RETURN NEW; END; +$$ LANGUAGE plpgsql; +CREATE TRIGGER set_updated_at_gai BEFORE UPDATE ON "public"."GithubAppInstallation" +FOR EACH ROW EXECUTE PROCEDURE set_updated_at(); +CREATE TRIGGER set_updated_at_gr BEFORE UPDATE ON "public"."GithubRepository" +FOR EACH ROW EXECUTE PROCEDURE set_updated_at();Also applies to: 30-31
1-46: Nit: Consistent “GitHub” casing in identifiers.Consider standardizing to “GitHub” across enum/table/index names for polish; no functional impact.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
apps/webapp/app/routes/_app.github.callback/route.tsx(1 hunks)apps/webapp/app/routes/_app.github.install/route.tsx(1 hunks)apps/webapp/app/routes/auth.github.callback.tsx(1 hunks)apps/webapp/app/utils.ts(1 hunks)internal-packages/database/prisma/migrations/20250829124138_add_gh_installation_schema/migration.sql(1 hunks)internal-packages/database/prisma/schema.prisma(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/webapp/app/routes/_app.github.callback/route.tsx
- internal-packages/database/prisma/schema.prisma
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/utils.tsapps/webapp/app/routes/_app.github.install/route.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/utils.tsapps/webapp/app/routes/_app.github.install/route.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/utils.tsapps/webapp/app/routes/_app.github.install/route.tsx
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead
Files:
apps/webapp/app/utils.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: myftija
PR: triggerdotdev/trigger.dev#2463
File: apps/webapp/app/routes/_app.github.callback/route.tsx:33-44
Timestamp: 2025-09-02T11:27:36.319Z
Learning: In the GitHub App installation callback flow in apps/webapp/app/routes/_app.github.callback/route.tsx, the install session cookie is not cleared after use due to interface limitations with redirectWithSuccessMessage/redirectWithErrorMessage not supporting custom headers. The maintainer accepts this design as the 1-hour cookie expiration provides sufficient protection against replay attacks.
Learnt from: myftija
PR: triggerdotdev/trigger.dev#2463
File: apps/webapp/app/services/gitHubSession.server.ts:31-36
Timestamp: 2025-09-02T11:18:06.562Z
Learning: In the GitHub App installation flow in apps/webapp/app/services/gitHubSession.server.ts, the redirectTo parameter stored in httpOnly session cookies is considered acceptable without additional validation by the maintainer, as the httpOnly cookie provides sufficient security for this use case.
📚 Learning: 2025-09-02T11:18:06.562Z
Learnt from: myftija
PR: triggerdotdev/trigger.dev#2463
File: apps/webapp/app/services/gitHubSession.server.ts:31-36
Timestamp: 2025-09-02T11:18:06.562Z
Learning: In the GitHub App installation flow in apps/webapp/app/services/gitHubSession.server.ts, the redirectTo parameter stored in httpOnly session cookies is considered acceptable without additional validation by the maintainer, as the httpOnly cookie provides sufficient security for this use case.
Applied to files:
apps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/_app.github.install/route.tsx
📚 Learning: 2025-09-02T11:27:36.319Z
Learnt from: myftija
PR: triggerdotdev/trigger.dev#2463
File: apps/webapp/app/routes/_app.github.callback/route.tsx:33-44
Timestamp: 2025-09-02T11:27:36.319Z
Learning: In the GitHub App installation callback flow in apps/webapp/app/routes/_app.github.callback/route.tsx, the install session cookie is not cleared after use due to interface limitations with redirectWithSuccessMessage/redirectWithErrorMessage not supporting custom headers. The maintainer accepts this design as the 1-hour cookie expiration provides sufficient protection against replay attacks.
Applied to files:
apps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/_app.github.install/route.tsx
🧬 Code graph analysis (2)
apps/webapp/app/routes/auth.github.callback.tsx (2)
apps/webapp/app/routes/auth.github.ts (1)
redirectCookie(29-32)apps/webapp/app/utils.ts (1)
sanitizeRedirectPath(13-42)
apps/webapp/app/routes/_app.github.install/route.tsx (5)
apps/webapp/app/utils.ts (1)
sanitizeRedirectPath(13-42)apps/webapp/app/services/session.server.ts (1)
requireUser(39-62)apps/webapp/app/db.server.ts (1)
$replica(103-106)apps/webapp/app/utils/pathBuilder.ts (1)
newOrganizationPath(96-98)apps/webapp/app/services/gitHubSession.server.ts (1)
createGitHubAppInstallSession(21-45)
⏰ 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: Analyze (javascript-typescript)
🔇 Additional comments (3)
apps/webapp/app/utils.ts (1)
13-24: Solid redirect hardening; clear allowlist semantics.Strict absolute-path checks and defaulting behavior meaningfully reduce open-redirect risk. Nice improvement.
apps/webapp/app/routes/auth.github.callback.tsx (1)
13-14: Good call: sanitize cookie-provided redirect before use.This closes an open-redirect vector without altering the existing flow.
internal-packages/database/prisma/migrations/20250829124138_add_gh_installation_schema/migration.sql (1)
44-44: Verify FK delete behavior for Organization.ON DELETE RESTRICT blocks org deletion if any installations exist. If product semantics prefer cascading cleanup, switch to CASCADE; otherwise, keep RESTRICT.
myftija
force-pushed
the
gh-app-installation-flow
branch
from
441d01a to
d07e605
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/webapp/app/services/gitHub.server.ts (2)
6-15: GitHub App instantiation LGTM.Gated by env and using env import correctly. appId as string is supported by Octokit.
59-91: Prefer full pagination; if intentionally capped, persist truncation metadata.Fetching only 3 pages can silently under-populate the cache for large installations. Use octokit.paginate to get all repos.
- const iterator = octokit.paginate.iterator(octokit.rest.apps.listReposAccessibleToInstallation, { - installation_id: installationId, - per_page: 100, - }); - const allRepos = []; - const maxPages = 3; - let pageCount = 0; - for await (const { data } of iterator) { - pageCount++; - allRepos.push(...data.repositories); - if (maxPages && pageCount >= maxPages) { /* warn */ break; } - } - return allRepos.map(/* ... */); + const repos = await octokit.paginate( + octokit.rest.apps.listReposAccessibleToInstallation, + { installation_id: installationId, per_page: 100 }, + (response) => response.data.repositories + ); + return repos.map(/* ...including BigInt conversion... */);If you keep the cap, set GithubAppInstallation.reposSyncTruncated and reposSyncedAt as suggested in the schema.
I can provide a differential updater to avoid delete+recreate for repos if you prefer.
🧹 Nitpick comments (5)
internal-packages/database/prisma/schema.prisma (3)
2259-2261: Cascade delete GithubAppInstallation when an Organization is deleted.Most relations to Organization use onDelete: Cascade. Add it here for consistency and to prevent orphaned installations.
- organization Organization @relation(fields: [organizationId], references: [id]) + organization Organization @relation(fields: [organizationId], references: [id], onDelete: Cascade, onUpdate: Cascade)
2248-2270: Track repo sync state to support partial/truncated caching.Given the fetch currently caps pages in code, add lightweight fields to indicate staleness.
model GithubAppInstallation { id String @id @default(cuid()) appInstallationId BigInt @unique targetId BigInt targetType String accountHandle String permissions Json? repositorySelection GithubRepositorySelection installedBy String? + reposSyncedAt DateTime? + reposSyncTruncated Boolean @default(false) organization Organization @relation(fields: [organizationId], references: [id], onDelete: Cascade, onUpdate: Cascade) organizationId String
2272-2290: Add helpful indexes for common lookups.If you’ll frequently resolve repos by full name or by GitHub ID independently, add indexes to avoid seq scans.
model GithubRepository { id String @id @default(cuid()) githubId BigInt name String fullName String htmlUrl String private Boolean defaultBranch String installation GithubAppInstallation @relation(fields: [installationId], references: [id], onDelete: Cascade, onUpdate: Cascade) installationId String createdAt DateTime @default(now()) updatedAt DateTime @updatedAt @@unique([installationId, githubId]) @@index([installationId]) + @@index([installationId, fullName]) + @@index([githubId]) }internal-packages/database/prisma/migrations/20250902133449_add_gh_installation_schema/migration.sql (2)
44-46: Use ON DELETE CASCADE for org → installation to prevent orphans.Matches the suggested Prisma relation change; update and re-generate migration.
-ALTER TABLE "public"."GithubAppInstallation" ADD CONSTRAINT "GithubAppInstallation_organizationId_fkey" FOREIGN KEY ("organizationId") REFERENCES "public"."Organization"("id") ON DELETE RESTRICT ON UPDATE CASCADE; +ALTER TABLE "public"."GithubAppInstallation" ADD CONSTRAINT "GithubAppInstallation_organizationId_fkey" FOREIGN KEY ("organizationId") REFERENCES "public"."Organization"("id") ON DELETE CASCADE ON UPDATE CASCADE;
40-43: Add indexes for common repo lookups.If queries hit by (installationId, fullName) or githubId, indexing here will help.
CREATE INDEX "GithubRepository_installationId_idx" ON "public"."GithubRepository"("installationId"); +CREATE INDEX "GithubRepository_installationId_fullName_idx" ON "public"."GithubRepository"("installationId","fullName"); +CREATE INDEX "GithubRepository_githubId_idx" ON "public"."GithubRepository"("githubId");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
apps/webapp/app/env.server.ts(1 hunks)apps/webapp/app/routes/_app.github.callback/route.tsx(1 hunks)apps/webapp/app/routes/_app.github.install/route.tsx(1 hunks)apps/webapp/app/routes/auth.github.callback.tsx(1 hunks)apps/webapp/app/routes/auth.github.ts(1 hunks)apps/webapp/app/services/gitHub.server.ts(1 hunks)apps/webapp/app/services/gitHubSession.server.ts(1 hunks)apps/webapp/app/utils.ts(1 hunks)apps/webapp/package.json(1 hunks)internal-packages/database/prisma/migrations/20250902133449_add_gh_installation_schema/migration.sql(1 hunks)internal-packages/database/prisma/migrations/migration_lock.toml(1 hunks)internal-packages/database/prisma/schema.prisma(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal-packages/database/prisma/migrations/migration_lock.toml
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/webapp/app/routes/_app.github.callback/route.tsx
- apps/webapp/package.json
- apps/webapp/app/routes/auth.github.callback.tsx
- apps/webapp/app/services/gitHubSession.server.ts
- apps/webapp/app/utils.ts
- apps/webapp/app/routes/_app.github.install/route.tsx
- apps/webapp/app/routes/auth.github.ts
- apps/webapp/app/env.server.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/app/services/gitHub.server.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/services/gitHub.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/services/gitHub.server.ts
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Files:
apps/webapp/app/services/gitHub.server.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead
Files:
apps/webapp/app/services/gitHub.server.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: myftija
PR: triggerdotdev/trigger.dev#2463
File: apps/webapp/app/routes/_app.github.callback/route.tsx:33-44
Timestamp: 2025-09-02T11:27:36.319Z
Learning: In the GitHub App installation callback flow in apps/webapp/app/routes/_app.github.callback/route.tsx, the install session cookie is not cleared after use due to interface limitations with redirectWithSuccessMessage/redirectWithErrorMessage not supporting custom headers. The maintainer accepts this design as the 1-hour cookie expiration provides sufficient protection against replay attacks.
Learnt from: myftija
PR: triggerdotdev/trigger.dev#2463
File: apps/webapp/app/services/gitHubSession.server.ts:31-36
Timestamp: 2025-09-02T11:18:06.562Z
Learning: In the GitHub App installation flow in apps/webapp/app/services/gitHubSession.server.ts, the redirectTo parameter stored in httpOnly session cookies is considered acceptable without additional validation by the maintainer, as the httpOnly cookie provides sufficient security for this use case.
📚 Learning: 2025-09-02T10:59:03.798Z
Learnt from: myftija
PR: triggerdotdev/trigger.dev#2463
File: apps/webapp/app/services/gitHub.server.ts:5-14
Timestamp: 2025-09-02T10:59:03.798Z
Learning: Octokit App constructor's appId parameter accepts both number and string types (number | string), so explicit conversion from string to number using Number() is not required when passing environment variables.
Applied to files:
apps/webapp/app/services/gitHub.server.ts
🧬 Code graph analysis (1)
apps/webapp/app/services/gitHub.server.ts (1)
apps/webapp/app/env.server.ts (1)
env(1176-1176)
⏰ 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). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
internal-packages/database/prisma/schema.prisma (1)
2243-2246: Enum looks good.Enum values match GitHub’s repository selection modes and align with code usage.
apps/webapp/app/services/gitHub.server.ts (1)
20-57: Unable to locate theGithubAppInstallationRepositorymodel in the Prisma schema – repositories are stored in theGithubRepositorymodel instead. The field in that model is namedgithubId, notgithubIdon a separate installation-repository table. Ensure you convertrepositoriesentries’idproperty from GitHub’s API to JSBigIntand assign it to thegithubIdfield in each createdGithubRepository.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/webapp/app/services/gitHub.server.ts (1)
127-134: Convert GitHub ID to BigInt to match Prisma schema
In apps/webapp/app/services/gitHub.server.ts (lines 127–134), wraprepo.idwithBigInt(String(...))to align with theBigIntfield in your Prisma schema and avoid precision/type errors:return allRepos.map((repo) => ({ - githubId: repo.id, + githubId: BigInt(String(repo.id)), name: repo.name, fullName: repo.full_name, htmlUrl: repo.html_url, private: repo.private, defaultBranch: repo.default_branch, }));
🧹 Nitpick comments (4)
apps/webapp/app/services/gitHub.server.ts (4)
72-75: Prefer findUnique over findFirst for unique key lookupsSlightly safer and communicates intent if appInstallationId is unique.
- const existingInstallation = await prisma.githubAppInstallation.findFirst({ - where: { appInstallationId: installationId }, - }); + const existingInstallation = await prisma.githubAppInstallation.findUnique({ + where: { appInstallationId: installationId }, + });
59-61: Fix comment: this function updates, it doesn’t linkMinor doc correction.
- * Links a GitHub App installation to a Trigger organization + * Updates a GitHub App installation’s metadata
82-100: Avoid writing suspendedAt to the same value (redundant/no-op)Either omit the field or set from the API if available (installation.suspended_at).
permissions: installation.permissions, - suspendedAt: existingInstallation?.suspendedAt, repositorySelection,If you want to sync from API:
// suspendedAt: installation.suspended_at ?? existingInstallation?.suspendedAt,
103-125: Don’t truncate repo pagination; fetch all or persist a “truncated” markerCapping at 3 pages can leave the repo cache incomplete and cause downstream mismatches. Either fetch all pages or persist a truncated flag and queue a background completion.
- const iterator = octokit.paginate.iterator(octokit.rest.apps.listReposAccessibleToInstallation, { - installation_id: installationId, - per_page: 100, - }); - - const allRepos = []; - const maxPages = 3; - let pageCount = 0; - - for await (const { data } of iterator) { - pageCount++; - allRepos.push(...data); - - if (maxPages && pageCount >= maxPages) { - logger.warn("GitHub installation repository fetch truncated", { - installationId, - maxPages, - totalReposFetched: allRepos.length, - }); - break; - } - } + const allRepos = await octokit.paginate( + octokit.rest.apps.listReposAccessibleToInstallation, + { installation_id: installationId, per_page: 100 } + );Note: Your current use of paginate.iterator and spreading data is correct; this change only simplifies and removes truncation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/webapp/app/routes/_app.github.callback/route.tsx(1 hunks)apps/webapp/app/services/gitHub.server.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webapp/app/routes/_app.github.callback/route.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/app/services/gitHub.server.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/services/gitHub.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/services/gitHub.server.ts
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Files:
apps/webapp/app/services/gitHub.server.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead
Files:
apps/webapp/app/services/gitHub.server.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: myftija
PR: triggerdotdev/trigger.dev#2463
File: apps/webapp/app/routes/_app.github.callback/route.tsx:33-44
Timestamp: 2025-09-02T11:27:36.319Z
Learning: In the GitHub App installation callback flow in apps/webapp/app/routes/_app.github.callback/route.tsx, the install session cookie is not cleared after use due to interface limitations with redirectWithSuccessMessage/redirectWithErrorMessage not supporting custom headers. The maintainer accepts this design as the 1-hour cookie expiration provides sufficient protection against replay attacks.
📚 Learning: 2025-09-02T10:59:03.798Z
Learnt from: myftija
PR: triggerdotdev/trigger.dev#2463
File: apps/webapp/app/services/gitHub.server.ts:5-14
Timestamp: 2025-09-02T10:59:03.798Z
Learning: Octokit App constructor's appId parameter accepts both number and string types (number | string), so explicit conversion from string to number using Number() is not required when passing environment variables.
Applied to files:
apps/webapp/app/services/gitHub.server.ts
📚 Learning: 2025-09-02T14:00:49.205Z
Learnt from: myftija
PR: triggerdotdev/trigger.dev#2463
File: apps/webapp/app/services/gitHub.server.ts:59-91
Timestamp: 2025-09-02T14:00:49.205Z
Learning: In Octokit's paginate plugin, the `octokit.paginate.iterator` method normalizes responses from endpoints like `listReposAccessibleToInstallation`. Even though the raw GitHub API returns `{ total_count, repositories, repository_selection }`, the paginate plugin extracts and returns just the repositories array directly in `response.data`. Therefore, spreading `...data` directly is correct, not `...data.repositories`.
Applied to files:
apps/webapp/app/services/gitHub.server.ts
📚 Learning: 2025-09-02T11:27:36.319Z
Learnt from: myftija
PR: triggerdotdev/trigger.dev#2463
File: apps/webapp/app/routes/_app.github.callback/route.tsx:33-44
Timestamp: 2025-09-02T11:27:36.319Z
Learning: In the GitHub App installation callback flow in apps/webapp/app/routes/_app.github.callback/route.tsx, the install session cookie is not cleared after use due to interface limitations with redirectWithSuccessMessage/redirectWithErrorMessage not supporting custom headers. The maintainer accepts this design as the 1-hour cookie expiration provides sufficient protection against replay attacks.
Applied to files:
apps/webapp/app/services/gitHub.server.ts
🧬 Code graph analysis (1)
apps/webapp/app/services/gitHub.server.ts (1)
apps/webapp/app/env.server.ts (1)
env(1176-1176)
⏰ 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). (23)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
apps/webapp/app/services/gitHub.server.ts (1)
6-15: GitHub App initialization looks good
- Correct env gating via env import (not process.env).
- Passing appId as string is fine; Octokit accepts number | string.
This PR implements the GitHub installation flow for the new Trigger.dev app.
An httpOnly state cookie is used to verify the installation session.
We cache some basic details about the installation repos to avoid querying the
GitHub API when we need to interact with the repos.