experiment(ask_gh): Support on demand indexing of GitHub repositories #785
Conversation
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
WalkthroughAdds an experimental AskGH feature: backend endpoint to add GitHub repos, a reusable repo-record builder, DB schema changes, index/job scheduling adjustments, frontend pages/components for repo display and indexing polling, and a feature-flagged layout toggle. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Client
participant Page as GitHubRepoPage
participant Backend as Backend API
participant GitHub as GitHub API
participant DB as Database
participant Indexer as RepoIndexManager
User->>Page: Navigate to /askgh/:owner/:repo
Page->>DB: Query repo by external_id/external_codeHostUrl (SINGLE_TENANT_ORG_ID)
alt repo not found
Page->>Backend: POST /api/experimental/add-github-repo {owner, repo}
Backend->>GitHub: Octokit fetch repository metadata
GitHub-->>Backend: Return repo metadata
Backend->>DB: Upsert Repo (createGitHubRepoRecord)
DB-->>Backend: Return repoId
Backend->>Indexer: Create INDEX job for repoId
Indexer-->>Backend: Return jobId
Backend-->>Page: { jobId, repoId }
else repo found
DB-->>Page: Return existing repoId
end
Page->>Page: Fetch repoInfo and language models
Page->>User: Render UI (RepoIndexedGuard + LandingPage)
alt repo indexed == false
User->>RepoIndexedGuard: Poll /api/repo-status/{repoId}
RepoIndexedGuard->>DB: getRepoInfo(repoId)
DB-->>RepoIndexedGuard: Return updated RepoInfo
RepoIndexedGuard->>User: Show indexing spinner until indexed
else indexed
RepoIndexedGuard->>User: Render chat UI
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ 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 |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In
`@packages/db/prisma/migrations/20260121012745_add_is_auto_cleanup_disabled_flag_to_repo_table/migration.sql`:
- Around line 1-2: The migration adds isAutoCleanupDisabled to the Repo table
but omits the new Repo.permissionSyncedAt column from the Prisma schema; update
this migration to also add a nullable timestamp column named
"permissionSyncedAt" on table "Repo" (use the DB type matching Prisma DateTime,
e.g., TIMESTAMP WITH TIME ZONE / timestamptz) so the database matches the schema
and queries won’t fail at runtime, and ensure the column is nullable with no
default to reflect when permissions are un-synced.
In
`@packages/web/src/app/`[domain]/askgh/[owner]/[repo]/components/repoIndexedGuard.tsx:
- Around line 33-35: RepoIndexedGuard currently returns null when isError is
true, producing a blank screen; replace that null with a user-facing error
state: render a UI similar to the loading state (icon/spinner, clear error
message like "Failed to check indexing status", and a retry action). In the
isError branch of RepoIndexedGuard, show the message and a retry button that
triggers the same polling/refetch function used for the happy/loading path (or
calls the existing refetch/startPolling handler used by the hook or context); if
no refetch function exists yet, wire one up to re-run the status request so the
retry button can re-attempt polling. Ensure accessibility by providing aria-live
or role="alert" for the message.
In
`@packages/web/src/app/`[domain]/askgh/[owner]/[repo]/components/repoStatusDisplay.tsx:
- Around line 79-89: getRepoStatus currently assumes fetch returns a successful
JSON body; update the function to check response.ok after the fetch and handle
non-OK responses by attempting to parse JSON if possible (or falling back to
text) and returning/throwing a ServiceError with meaningful status and message;
also wrap response.json() in try/catch to handle invalid JSON and convert
parsing failures into a ServiceError. Ensure you update the return path of
getRepoStatus (which currently returns RepoInfo | ServiceError) to return a
consistent ServiceError object on non-OK or parse errors and keep returning
RepoInfo on success.
- Around line 34-37: The component currently returns null when isError is true,
producing a blank screen; update the error branch in the RepoStatusDisplay
component (the block checking isError) to render a minimal error state instead
of null — e.g., return a small accessible UI element (a div or span with
role="alert") that shows a user-facing message like "Failed to load repository
status" and, if available, an error retry button or link; ensure the element is
styled/structured consistently with other small notices in the app and includes
any available error details for debugging.
- Around line 54-71: The repo name is inserted raw into the regex-based
SearchBar default (defaults.query = `repo:^${repoInfo.name}$`), which allows
regex metacharacters in repoInfo.name to alter matches; fix by escaping regex
metacharacters before interpolation (e.g., add or use an escapeRegExp utility
and call it on repoInfo.name) and then build the query with the escaped value so
defaults.query becomes `repo:^${escapedName}$`; update references in
repoStatusDisplay (where repoInfo.name is used) and ensure the SearchBar
defaults.query uses the escapedName.
In `@packages/web/src/app/`[domain]/askgh/[owner]/[repo]/page.tsx:
- Around line 20-44: The current flow lets two requests pass
prisma.repo.findFirst and both call addGithubRepo, causing prisma.repo.create to
throw a unique-constraint error; update addGithubRepo to perform an upsert (use
prisma.repo.upsert with the unique key on
external_id/external_codeHostUrl/orgId) or wrap the existing prisma.repo.create
in a try/catch that detects Prisma unique-constraint errors and then performs
prisma.repo.findUnique/findFirst to return the existing repo id; ensure the
function still returns the same shape (response.repoId) and keep
ServiceErrorException usage for other failures so callers like the code that
awaits addGithubRepo continue to get a consistent result.
In `@packages/web/src/app/api/repo-status/`[repoId]/route.ts:
- Around line 10-16: The current validation uses parseInt on params.repoId
(variables: params, repoId, repoIdNum, parseInt) which allows partial strings
like "123abc" or hex "0x10"; update the check to first ensure repoId is a
strictly decimal integer string (e.g., match /^\d+$/) and only then convert to a
Number/parseInt and use that numeric value, returning the 400 response if the
regex fails; apply the same change to the other occurrence in the image route
file as well.
🧹 Nitpick comments (3)
packages/web/src/app/[domain]/askgh/[owner]/[repo]/components/repoIndexedGuard.tsx (1)
58-67: Guard against non‑JSON responses from/api/repo-status.
If the response isn’t JSON (e.g., unexpected 500 body),response.json()throws and you lose context. Catch parse errors and throw a clearer message.♻️ Suggested refactor
- const result = await fetch( - `/api/repo-status/${repoId}`, - { - method: 'GET', - headers: { - 'Content-Type': 'application/json', - }, - } - ).then(response => response.json()); - return result as RepoInfo | ServiceError; + const response = await fetch( + `/api/repo-status/${repoId}`, + { + method: 'GET', + headers: { + 'Content-Type': 'application/json', + }, + } + ); + const result = await response.json().catch(() => { + throw new Error(`Failed to parse repo-status response (${response.status})`); + }); + return result as RepoInfo | ServiceError;packages/web/src/app/[domain]/askgh/[owner]/[repo]/api.ts (1)
10-19: Drop unusedjobsinclude to reduce query work.
The relation isn’t used in theRepoInfomapping; removing it avoids extra join/order overhead.♻️ Suggested refactor
- const repo = await prisma.repo.findUnique({ - where: { id: repoId }, - include: { - jobs: { - orderBy: { - createdAt: 'desc', - }, - take: 1, - }, - }, - }); + const repo = await prisma.repo.findUnique({ + where: { id: repoId }, + });packages/web/src/app/[domain]/askgh/[owner]/[repo]/components/repoStatusDisplay.tsx (1)
22-27: Align polling comment with actual field.The comment mentions
indexedAt, but the logic checksisIndexed. Please update the comment to avoid confusion.♻️ Proposed fix
- // If repo has been indexed before (indexedAt is not null), stop polling + // If repo is indexed, stop polling
...isma/migrations/20260121012745_add_is_auto_cleanup_disabled_flag_to_repo_table/migration.sql
Show resolved
Hide resolved
packages/web/src/app/[domain]/askgh/[owner]/[repo]/components/repoIndexedGuard.tsx
Show resolved
Hide resolved
packages/web/src/app/[domain]/askgh/[owner]/[repo]/components/repoStatusDisplay.tsx
Outdated
Show resolved
Hide resolved
packages/web/src/app/[domain]/askgh/[owner]/[repo]/components/repoStatusDisplay.tsx
Outdated
Show resolved
Hide resolved
packages/web/src/app/[domain]/askgh/[owner]/[repo]/components/repoStatusDisplay.tsx
Outdated
Show resolved
Hide resolved
brendan-kellam
changed the title
brendan-kellam
changed the title
Contributor
|
@brendan-kellam your pull request is missing a changelog! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/backend/src/api.ts`:
- Line 38: All API routes in api.ts are registered without authentication; add a
centralized auth middleware and apply it to every route (e.g., wrap app.post
registrations) so handlers like experimental_addGithubRepo, syncConnection, and
indexRepo are protected; implement or reuse a function such as
ensureAuthenticated(req,res,next) and attach it to each route declaration (or to
the router/app globally) and, if feature gating is required, add a feature-flag
check (e.g., isFeatureEnabled('experimental') or env guard) inside either the
middleware or the individual handlers to block access when the flag is off.
In `@packages/web/src/app/`[domain]/askgh/layout.tsx:
- Around line 4-16: AskGHLayout currently accesses params synchronously which
breaks in Next.js 15; make the layout async, change the params type to a Promise
(e.g. params: Promise<{ domain: string }>), await params inside AskGHLayout to
extract domain (const { domain } = await params) and then use domain in the
redirect and return; update the AskGHLayout function signature to async and
adjust any type annotations accordingly.
♻️ Duplicate comments (1)
packages/web/src/app/api/(server)/repo-status/[repoId]/route.ts (1)
10-16: Tighten repoId validation to reject partial/hex inputs.
parseIntaccepts values like123abc(parses to123) or0x10(parses to16), which could lead to unintended repo lookups. Use strict numeric validation.🔧 Suggested fix
- const repoIdNum = parseInt(repoId); - - if (isNaN(repoIdNum)) { + if (!/^\d+$/.test(repoId)) { + return new Response("Invalid repo ID", { status: 400 }); + } + const repoIdNum = Number(repoId); + if (!Number.isSafeInteger(repoIdNum)) { return new Response("Invalid repo ID", { status: 400 }); }
🧹 Nitpick comments (1)
packages/backend/src/api.ts (1)
110-114: Consider authenticated Octokit to avoid low rate limits / missing private repos.
If this endpoint will be used beyond trivial testing, unauthenticated requests will quickly hit GitHub’s public rate limits and won’t allow private repo lookups. Consider injecting a token via env config.🔧 Example
- const octokit = new Octokit(); + const octokit = new Octokit({ + auth: process.env.GITHUB_TOKEN, + });
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/backend/src/api.ts`:
- Around line 100-103: The schema for request validation (the const schema =
z.object({...}).strict()) currently allows empty or whitespace-only owner/repo
strings; update the owner and repo validators to z.string().trim().min(1) so
they reject blank input before calling the GitHub API, keeping the surrounding
z.object(...) and .strict() intact; ensure both the owner and repo properties
are changed.
- Around line 111-137: Replace the unauthenticated, unhandled GitHub call by
instantiating Octokit with process.env.GITHUB_TOKEN when present (use the
Octokit constructor) and wrap the octokit.rest.repos.get(...) call in a
try/catch; on error inspect err.status (or err.statusCode) and return matching
HTTP responses for 404 (res.status(404)), 403 (res.status(403)), and 429
(res.status(429)) with a helpful message, and for other errors return a 502/500
as appropriate; keep the subsequent createGitHubRepoRecord(...),
prisma.repo.upsert(...) and repoIndexManager.createJobs(...) logic but only run
them after a successful repo fetch.
♻️ Duplicate comments (1)
packages/backend/src/api.ts (1)
37-39: Endpoint gating still missing (previously flagged).This experimental route is still registered without auth/feature-flag protection, matching the earlier review finding.
Comment on lines
+100
to
+103
| const schema = z.object({ | ||
| owner: z.string(), | ||
| repo: z.string(), | ||
| }).strict(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file around the specified lines
cat -n packages/backend/src/api.ts | sed -n '95,110p'Repository: sourcebot-dev/sourcebot
Length of output: 698
🏁 Script executed:
# Let's also check the full context of this schema usage
cat -n packages/backend/src/api.ts | sed -n '90,120p'Repository: sourcebot-dev/sourcebot
Length of output: 1265
🌐 Web query:
Zod string trim() min() validation API documentation
💡 Result:
In Zod, both .trim() and .min() are chainable methods on z.string() schemas:
.trim(): a transformation that trims leading/trailing whitespace. Example shown in the official “Strings” guide:z.string().trim();(commented “trim whitespace”). [1].min(length, options?): a validation that enforces a minimum string length. Example:z.string().min(5);and with a custom message:z.string().min(5, { message: "Must be 5 or more characters long" });. [1]
Typical usage to require a non-empty string after trimming:
import { z } from "zod";
const schema = z.string().trim().min(1, { message: "Required" });(Chaining is the standard Zod API style—e.g., z.string().min(5).max(10).toLowerCase()). [2]
Sources: Zod strings guide [1], Zod v4 package docs [2].
Tighten owner/repo validation.
Empty strings (including whitespace-only strings) currently pass validation and would fail when passed to the GitHub API. Add .trim().min(1) to both fields to validate early and prevent unnecessary API calls.
🔧 Suggested fix
- const schema = z.object({
- owner: z.string(),
- repo: z.string(),
- }).strict();
+ const schema = z.object({
+ owner: z.string().trim().min(1),
+ repo: z.string().trim().min(1),
+ }).strict();📝 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
| const schema = z.object({ | |
| owner: z.string(), | |
| repo: z.string(), | |
| }).strict(); | |
| const schema = z.object({ | |
| owner: z.string().trim().min(1), | |
| repo: z.string().trim().min(1), | |
| }).strict(); |
🤖 Prompt for AI Agents
In `@packages/backend/src/api.ts` around lines 100 - 103, The schema for request
validation (the const schema = z.object({...}).strict()) currently allows empty
or whitespace-only owner/repo strings; update the owner and repo validators to
z.string().trim().min(1) so they reject blank input before calling the GitHub
API, keeping the surrounding z.object(...) and .strict() intact; ensure both the
owner and repo properties are changed.
Comment on lines
+111
to
+137
| const octokit = new Octokit(); | ||
| const response = await octokit.rest.repos.get({ | ||
| owner: parsed.data.owner, | ||
| repo: parsed.data.repo, | ||
| }); | ||
|
|
||
| const record = createGitHubRepoRecord({ | ||
| repo: response.data, | ||
| hostUrl: 'https://github.com', | ||
| isAutoCleanupDisabled: true, | ||
| }); | ||
|
|
||
| const repo = await this.prisma.repo.upsert({ | ||
| where: { | ||
| external_id_external_codeHostUrl_orgId: { | ||
| external_id: record.external_id, | ||
| external_codeHostUrl: record.external_codeHostUrl, | ||
| orgId: SINGLE_TENANT_ORG_ID, | ||
| } | ||
| }, | ||
| update: record, | ||
| create: record, | ||
| }); | ||
|
|
||
| const [jobId ] = await this.repoIndexManager.createJobs([repo], RepoIndexingJobType.INDEX); | ||
|
|
||
| res.status(200).json({ jobId, repoId: repo.id }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if file exists and view the content around lines 111-137
if [ -f "packages/backend/src/api.ts" ]; then
echo "File found. Checking lines 111-137:"
sed -n '111,137p' packages/backend/src/api.ts
echo ""
echo "Broader context (lines 100-150):"
wc -l packages/backend/src/api.ts
else
echo "File not found. Searching for api.ts files:"
find . -name "api.ts" -type f 2>/dev/null | head -20
fiRepository: sourcebot-dev/sourcebot
Length of output: 1094
🏁 Script executed:
# Check for imports and logger availability
if [ -f "packages/backend/src/api.ts" ]; then
echo "=== Imports and class context ==="
head -50 packages/backend/src/api.ts
fiRepository: sourcebot-dev/sourcebot
Length of output: 1925
🏁 Script executed:
# Search for existing error handling patterns in the codebase
echo "=== Searching for octokit usage patterns ==="
rg -A 5 "new Octokit" --type ts packages/backend/src/Repository: sourcebot-dev/sourcebot
Length of output: 805
🌐 Web query:
Octokit JavaScript error handling status codes 404 403 429 rate limit
💡 Result:
In Octokit JS, GitHub API failures are typically thrown as a RequestError with:
error.status= HTTP status codeerror.response={ status, url, headers, data }(when a response exists) (npm.io)
How to interpret / handle the status codes
404 Not Found
- Resource/route doesn’t exist or GitHub is intentionally hiding a private/inaccessible resource behind a 404 (common pattern). Handle as “missing or not accessible,” and double-check owner/repo names, endpoint, and auth scopes.
403 Forbidden
- Usually “insufficient permissions / policy” or rate limiting (GitHub may return 403 or 429 when you exceed primary/secondary rate limits). (docs.github.com)
- For rate limiting, inspect headers and message (see below).
429 Too Many Requests
- Rate limiting (primary or secondary). GitHub documents that exceeding rate limits can return 403 or 429. (docs.github.com)
Rate limit signals to check (403/429)
GitHub’s guidance when rate-limited: (docs.github.com)
x-ratelimit-remaining: 0⇒ wait untilx-ratelimit-reset(UTC epoch seconds)retry-after(if present) ⇒ wait that many seconds (often for secondary rate limits)- Secondary rate limit responses include a message indicating you exceeded a secondary rate limit; if no
retry-after, GitHub recommends waiting at least ~60 seconds, then backing off more if it persists. (docs.github.com)
Practical Octokit pattern
try {
await octokit.request("GET /repos/{owner}/{repo}", { owner, repo });
} catch (error) {
if (!error.status) throw error; // not an Octokit HTTP error
const status = error.status;
const headers = error.response?.headers || {};
const message = error.response?.data?.message;
if (status === 404) {
// not found OR not accessible
} else if (status === 403 || status === 429) {
const remaining = headers["x-ratelimit-remaining"];
const reset = headers["x-ratelimit-reset"];
const retryAfter = headers["retry-after"];
// decide whether this is rate limit vs permissions by checking remaining/message
} else {
// other HTTP errors
}
}Best practice: let Octokit throttle for you
If you want automatic handling/retries for rate limits, use @octokit/plugin-throttling, which provides hooks like onRateLimit and onSecondaryRateLimit and computes retryAfter from retry-after / x-ratelimit-reset. (github.com)
Citations:
- 1: https://npm.io/package/%40octokit/request-error
- 2: https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api
- 3: https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api
- 4: https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api
- 5: https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api
- 6: https://github.com/octokit/plugin-throttling.js/
Handle GitHub API failures explicitly and prefer authenticated Octokit.
The octokit.rest.repos.get() call at lines 116–119 has no error handling. GitHub API failures will surface as 500s instead of appropriate status codes, and unauthenticated calls are rate-limited to 60 requests/hour (vs. 5000 authenticated), which will cause silent failures. Use a configured GITHUB_TOKEN when available and catch specific status codes (404, 403, 429) to return correct HTTP responses.
🛠️ Proposed handling
- const octokit = new Octokit();
- const response = await octokit.rest.repos.get({
- owner: parsed.data.owner,
- repo: parsed.data.repo,
- });
+ const octokit = process.env.GITHUB_TOKEN
+ ? new Octokit({ auth: process.env.GITHUB_TOKEN })
+ : new Octokit();
+
+ let response;
+ try {
+ response = await octokit.rest.repos.get({
+ owner: parsed.data.owner,
+ repo: parsed.data.repo,
+ });
+ } catch (err: any) {
+ const status = err?.status;
+ if (status === 404) {
+ res.status(404).json({ error: 'GitHub repo not found' });
+ return;
+ }
+ if (status === 401 || status === 403) {
+ res.status(403).json({ error: 'GitHub access denied' });
+ return;
+ }
+ if (status === 429) {
+ res.status(429).json({ error: 'GitHub rate limit exceeded' });
+ return;
+ }
+ logger.error({ err }, 'GitHub API request failed');
+ res.status(502).json({ error: 'GitHub API error' });
+ return;
+ }🤖 Prompt for AI Agents
In `@packages/backend/src/api.ts` around lines 111 - 137, Replace the
unauthenticated, unhandled GitHub call by instantiating Octokit with
process.env.GITHUB_TOKEN when present (use the Octokit constructor) and wrap the
octokit.rest.repos.get(...) call in a try/catch; on error inspect err.status (or
err.statusCode) and return matching HTTP responses for 404 (res.status(404)),
403 (res.status(403)), and 429 (res.status(429)) with a helpful message, and for
other errors return a 502/500 as appropriate; keep the subsequent
createGitHubRepoRecord(...), prisma.repo.upsert(...) and
repoIndexManager.createJobs(...) logic but only run them after a successful repo
fetch.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 by CodeRabbit
New Features
Database
✏️ Tip: You can customize this high-level summary in your review settings.