fix: defer pr issue links backfill#171
Merged
Merged
Conversation
Root cause of the Explorer page's "stuck loading skeleton" (Cloudflare 524 timeouts at the origin): `backfillPrIssueLinksIfNeeded` is called inline by 8 API routes (issues/route, pulls/route, issues-meta, related-issues, related-prs, gt/repos/[owner]/[name], etc.). On a cold path where `pr_issue_links` is empty for the repo (e.g. after PR #137's schema-purge migration), the function synchronously: 1. SELECTs every PR + title + body for the repo 2. Runs `extractLinkedIssues` regex over each PR's body 3. Writes resulting links in a single sqlite transaction For a repo with hundreds+ of PRs this can stall the Node event loop for tens of seconds. Cloudflare gives up at 100s and returns 524 to the client; the consumer in RepoExplorer.tsx treats !r.ok as fatal and leaves the Issues / Pull Requests tabs stuck on their skeleton. Fix: split the backfill into a request-path entry point that gates + defers, and a separate `runPrIssueLinksBackfill` that does the heavy work. The entry point now: - Returns immediately when the gate count > 0 (existing fast path, unchanged behaviour for steady state). - On cold start, schedules the backfill via setImmediate and returns 0 to the caller. A per-repo in-flight Set dedupes concurrent cold requests so we don't pile up serial writer-tx queues. The first request after a purge / cold start returns empty linked- issues / linked-PRs enrichment, but the route responds in milliseconds instead of timing out. Subsequent requests (after the deferred backfill commits) get full enrichment. Companion to df1f09c + 2fa2d13 + 99df6b1 on this branch which catch *throws* — those didn't help here because the underlying handler was *hanging*, not erroring. This is the actual root-cause patch.
CodeRabbit caught a real edge case in #168: my cold-path check is `if (existingCount > 0) return existingCount;`. For repos that legitimately have zero linked issues (brand-new repo, or every PR genuinely has no `Closes #N` references), `existingCount === 0` is ALWAYS true → every request to one of the 8 calling routes schedules another backfill → every request re-loads all PRs + bodies and regex-extracts them, even though the previous backfill already ran and confirmed there's nothing to write. In-flight Set only dedupes concurrent schedules, not subsequent ones. Fix: add a persistent marker column `pr_issue_links_backfilled_at` on repo_meta (idempotent migration in db.ts alongside the existing ALTER TABLE backfill columns). The cold-path check now reads the marker too — once set, the repo is considered backfilled even with zero rows. Marker is written by the deferred worker on successful completion only; failures intentionally leave it null so the next request retries. Real cost without this fix: every request for a linkless repo runs the full PR scan in the background. Defer keeps it off the request path so users don't see latency, but it's wasted CPU per-request and amplifies under traffic. Follow-up to 725340e.
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (18)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (14)
📝 WalkthroughWalkthroughThis PR consolidates repository handling to live Gittensor-listed repos by removing user-repo augmentation across backend APIs, frontend components, and polling infrastructure. It introduces cache pruning for removed upstream repos, adds API endpoint guards to block untracked repos, and simplifies query logic to use only live repo weights. ChangesUser-Repo Integration Removal and Live-Repo Unification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/app/api/all-repos/route.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/app/api/issues/route.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. src/app/api/poller-status/route.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
Comment |
MkDev11
force-pushed
the
fix/defer-pr-issue-links-backfill
branch
from
38f458a to
1b0d56f
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/docs/page.tsx (1)
483-483:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused
Pillcomponent to clear lint warning.
Pillis defined but never used, and this is currently failing lint in CI. Please delete it (or use it) before merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/docs/page.tsx` at line 483, The Pill component (function Pill with signature function Pill({ children }: { children: React.ReactNode })) is declared but unused and causing lint failures; remove the entire Pill function declaration (and any related unused imports/exports) from the file, or alternatively replace its removal by using the Pill component where intended—ensure no remaining references or unused props (children) remain so lint passes.src/components/RepoExplorer.tsx (1)
554-580:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle selected repo fallback when live list removes a repo.
If the current
selectedrepo disappears fromallReposafter refresh, the effect keeps a stale selection instead of switching to a valid live repo.Suggested fix
useEffect(() => { const urlRepo = searchParams?.get('repo') ?? null; setSelected((prev) => { + const prevStillLive = allRepos.some((x) => x.fullName === prev.fullName); + const firstLive = allRepos[0]; + // Prefer the URL-specified repo when present. if (urlRepo) { - if (prev.fullName === urlRepo) return prev; + if (prev.fullName === urlRepo && prevStillLive) return prev; const found = allRepos.find((x) => x.fullName === urlRepo); - if (!found) return prev; + if (!found) return prevStillLive ? prev : (firstLive ?? prev); selectedFromUrlRef.current = true; return found; } // No `?repo=` and we're still on the placeholder: promote the // first real repo from the live list so the page has something // to render once `/api/sn74-repos` lands. - if (!prev.fullName && allRepos.length > 0) return allRepos[0]; + if ((!prev.fullName || !prevStillLive) && firstLive) return firstLive; return prev; }); }, [searchParams, allRepos]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/RepoExplorer.tsx` around lines 554 - 580, The effect that sets selected (inside RepoExplorer's useEffect using setSelected, searchParams, allRepos and selectedFromUrlRef) doesn't handle the case where the current prev.fullName is no longer present in allRepos, so the UI keeps a stale selection; update the setter logic so that if there is no ?repo= (urlRepo falsy) and prev.fullName is non-empty but cannot be found in allRepos, you promote a valid fallback (e.g., allRepos[0]) instead of returning prev; ensure the existing behavior for URL-specified repo remains (set selectedFromUrlRef.current = true when found) and keep the branch that promotes the first live repo when prev was the placeholder.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/refresh.ts`:
- Around line 323-371: Normalize the incoming repoFullName (e.g., const
normalized = repoFullName.toLowerCase().trim()) at the top of
backfillPrIssueLinksIfNeeded and use that normalized value for all gating, SQL
lookups (COUNT and repo_meta SELECT), the inFlightLinksBackfill set/has/delete
calls, the runPrIssueLinksBackfill invocation inside setImmediate, and the
INSERT/UPDATE of repo_meta so casing differences can't cause duplicate backfills
or fragmented repo_meta rows; ensure all comparisons and writes consistently use
the same normalized identifier.
In `@src/lib/repos-server.ts`:
- Around line 261-263: The early return in pruneCachedDataToLiveRepos when
liveKeys.length === 0 prevents pruning cached tables and leaves stale rows
(e.g., repo_weights) when upstream yields an empty repo set; remove that early
return and instead handle the empty liveKeys case explicitly in
pruneCachedDataToLiveRepos: when liveKeys is empty run the same delete logic but
without a WHERE IN clause (i.e., delete all rows from repo_weights and related
cache tables or use a query that matches no live keys), ensure removed is
incremented for the rows deleted, and apply the same empty-set handling to the
other pruning path referenced around the repo_weights logic (the logic mentioned
at the other location noted in the review) so both paths consistently clear
cache when liveKeys is empty.
---
Outside diff comments:
In `@src/app/docs/page.tsx`:
- Line 483: The Pill component (function Pill with signature function Pill({
children }: { children: React.ReactNode })) is declared but unused and causing
lint failures; remove the entire Pill function declaration (and any related
unused imports/exports) from the file, or alternatively replace its removal by
using the Pill component where intended—ensure no remaining references or unused
props (children) remain so lint passes.
In `@src/components/RepoExplorer.tsx`:
- Around line 554-580: The effect that sets selected (inside RepoExplorer's
useEffect using setSelected, searchParams, allRepos and selectedFromUrlRef)
doesn't handle the case where the current prev.fullName is no longer present in
allRepos, so the UI keeps a stale selection; update the setter logic so that if
there is no ?repo= (urlRepo falsy) and prev.fullName is non-empty but cannot be
found in allRepos, you promote a valid fallback (e.g., allRepos[0]) instead of
returning prev; ensure the existing behavior for URL-specified repo remains (set
selectedFromUrlRef.current = true when found) and keep the branch that promotes
the first live repo when prev was the placeholder.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b814b0ec-0f76-4f3d-9dd3-4d5cd3fd248e
📒 Files selected for processing (19)
src/app/api/all-repos/route.tssrc/app/api/issues/route.tssrc/app/api/poller-status/route.tssrc/app/api/pulls/route.tssrc/app/api/repo-activity/route.tssrc/app/api/repos/[owner]/[name]/issues/route.tssrc/app/api/repos/[owner]/[name]/pulls/route.tssrc/app/docs/page.tsxsrc/app/issues/page.tsxsrc/app/pulls/page.tsxsrc/components/IssuesTable.tsxsrc/components/NewIssuesWatcher.tsxsrc/components/RepoExplorer.tsxsrc/lib/api-utils.tssrc/lib/db.tssrc/lib/poller.tssrc/lib/refresh.tssrc/lib/repos-server.tssrc/lib/use-sn74-repos.ts
💤 Files with no reviewable changes (1)
- src/lib/api-utils.ts
MkDev11
changed the title
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 by CodeRabbit
New Features
Documentation
Refactor