Skip to content

perf: rewrite gh CLI subprocesses to use native fetch and SQLite caching #4

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
OctavianTocan wants to merge 1 commit into
base: main
Choose a base branch
from
Open

perf: rewrite gh CLI subprocesses to use native fetch and SQLite caching #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions src/__tests__/integration.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,29 +76,29 @@ describe("fetchOpenPRs (real gh CLI)", () => {

describe("fetchRepoPRs (real gh CLI)", () => {
test("returns PRs for a known repo with branch info", async () => {
// OctavianTocan/to-do-app has many open PRs
const prs = await fetchRepoPRs("OctavianTocan/to-do-app")
// cli/cli has many open PRs
const prs = await fetchRepoPRs("cli/cli")
expect(Array.isArray(prs)).toBe(true)
expect(prs.length).toBeGreaterThan(0)

const pr = prs[0]
expect(pr.repo).toBe("OctavianTocan/to-do-app")
expect(pr.repo).toBe("cli/cli")
expect(pr.headRefName).toBeTruthy()
expect(pr.baseRefName).toBeTruthy()
expect(pr.number).toBeGreaterThan(0)
expect(pr.url).toContain("github.com/OctavianTocan/to-do-app/pull/")
expect(pr.url).toContain("github.com/cli/cli/pull/")
})

test("body is truncated to max 80 chars single line", async () => {
const prs = await fetchRepoPRs("OctavianTocan/to-do-app")
const prs = await fetchRepoPRs("cli/cli")
for (const pr of prs) {
expect(pr.body.length).toBeLessThanOrEqual(80)
expect(pr.body).not.toContain("\n")
}
})

test("all PRs have the correct repo field set", async () => {
const repo = "OctavianTocan/to-do-app"
const repo = "cli/cli"
const prs = await fetchRepoPRs(repo)
for (const pr of prs) {
expect(pr.repo).toBe(repo)
Expand Down Expand Up @@ -193,7 +193,7 @@ describe("formatStackedTitle", () => {

describe("end-to-end: fetch -> detect -> format", () => {
test("full pipeline works for a real repo", async () => {
const prs = await fetchRepoPRs("OctavianTocan/to-do-app")
const prs = await fetchRepoPRs("cli/cli")
expect(prs.length).toBeGreaterThan(0)

const stacks = detectStacks(prs)
Expand Down
142 changes: 90 additions & 52 deletions src/commands/ls.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,18 @@
import { useState, useEffect, useMemo, useCallback } from "react"
import { useState, useEffect, useMemo, useCallback, useDeferredValue, useRef } from "react"
import { useKeyboard, useRenderer, useTerminalDimensions } from "@opentui/react"
import { PRTable } from "../components/pr-table"
import { Spinner } from "../components/spinner"
import { SkeletonList } from "../components/skeleton"
import { StatusView } from "../components/status-view"
import { fetchOpenPRs, getCurrentRepo, fetchPRDetails, submitPRReview, postPRComment, replyToReviewComment, resolveReviewThread } from "../lib/github"
import { fetchOpenPRs, getCurrentRepo, batchFetchPRDetails, submitPRReview, postPRComment, replyToReviewComment, resolveReviewThread } from "../lib/github"
import { shortRepoName } from "../lib/format"
import { PreviewPanel } from "../components/preview-panel"
import { usePanel } from "../hooks/usePanel"
import { detectPRState, compareByUrgency } from "../lib/pr-lifecycle"
import { findNextUnresolvedCommentIndex, getCodeCommentThreadStats, markThreadResolved } from "../lib/review-threads"
import type { PullRequest, Density, PRDetails, PanelTab, PRPanelData, PRLifecycleInfo } from "../lib/types"
import { groupByRepo, groupByStack, groupByRepoAndStack, type GroupMode, type GroupedData } from "../lib/grouping"
import { getCachedPRs, cachePRs } from "../lib/db"

interface LsCommandProps {
author?: string
Expand Down Expand Up @@ -68,22 +69,37 @@ export function LsCommand({ author, repoFilter: initialRepoFilter }: LsCommandPr
}, [initialRepoFilter])

useEffect(() => {
const cached = getCachedPRs()
if (cached.length > 0) {
const sorted = [...cached].sort((a, b) => {
const repoCompare = a.repo.localeCompare(b.repo)
if (repoCompare !== 0) return repoCompare
return a.number - b.number
})
setAllPRs(sorted)
setLoading(false)
}

let mounted = true
async function load() {
try {
let results = await fetchOpenPRs(author, setLoadingStatus)
if (!mounted) return
results.sort((a, b) => {
const repoCompare = a.repo.localeCompare(b.repo)
if (repoCompare !== 0) return repoCompare
return a.number - b.number
})
setAllPRs(results)
cachePRs(results)
} catch (e) {
setError(e instanceof Error ? e.message : "Failed to fetch PRs")
if (!cached.length) setError(e instanceof Error ? e.message : "Failed to fetch PRs")
} finally {
setLoading(false)
if (mounted) setLoading(false)
}
}
load()
return () => { mounted = false }
}, [author])

// Get unique repos and authors for cycling
Expand All @@ -99,6 +115,8 @@ export function LsCommand({ author, repoFilter: initialRepoFilter }: LsCommandPr

const [authorFilter, setAuthorFilter] = useState<string | null>(null)

const deferredSearchQuery = useDeferredValue(searchQuery)
Copy link
Copy Markdown

@sourcery-ai sourcery-ai bot Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): useDeferredValue is used but not imported in this module.

This hook isn’t imported in this file, so deferredSearchQuery will fail at runtime. Please add useDeferredValue to the React import (e.g. import React, { ..., useDeferredValue } from "react").


const filteredPRs = useMemo(() => {
let prs = allPRs
// Repo filter
Expand All @@ -113,101 +131,116 @@ export function LsCommand({ author, repoFilter: initialRepoFilter }: LsCommandPr
if (statusFilter === "open") prs = prs.filter((pr) => !pr.isDraft)
if (statusFilter === "draft") prs = prs.filter((pr) => pr.isDraft)
// Search filter
if (searchQuery) {
const q = searchQuery.toLowerCase()
if (deferredSearchQuery) {
const q = deferredSearchQuery.toLowerCase()
prs = prs.filter((pr) =>
pr.title.toLowerCase().includes(q) ||
shortRepoName(pr.repo).toLowerCase().includes(q) ||
pr.repo.toLowerCase().includes(q) ||
String(pr.number).includes(q)
)
}
// Sort
prs = [...prs].sort((a, b) => {
return prs
}, [allPRs, repoFilter, authorFilter, statusFilter, deferredSearchQuery])

const urgencyMap = useMemo(() => {
const map = new Map<string, number>()
for (const pr of filteredPRs) {
const details = detailsMap.get(pr.url) ?? null
const state = detectPRState(pr, details)
map.set(pr.url, state.urgency)
}
return map
}, [filteredPRs, detailsMap])

const sortedPRs = useMemo(() => {
const copy = [...filteredPRs]
const timestamps = sortMode === "age"
? new Map(copy.map(pr => [pr.url, new Date(pr.createdAt).getTime()]))
: null

return copy.sort((a, b) => {
switch (sortMode) {
case "attention": {
// Sort by lifecycle urgency score (computed below in lifecycleMap)
const aState = detailsMap.has(a.url) ? detectPRState(a, detailsMap.get(a.url)!) : detectPRState(a, null)
const bState = detailsMap.has(b.url) ? detectPRState(b, detailsMap.get(b.url)!) : detectPRState(b, null)
return compareByUrgency(
{ urgency: aState.urgency, createdAt: a.createdAt },
{ urgency: bState.urgency, createdAt: b.createdAt },
)
const aUrg = urgencyMap.get(a.url) ?? 0
const bUrg = urgencyMap.get(b.url) ?? 0
if (aUrg !== bUrg) return bUrg - aUrg
return new Date(b.createdAt).getTime() - new Date(a.createdAt).getTime()
}
case "repo": {
const rc = a.repo.localeCompare(b.repo)
return rc !== 0 ? rc : a.number - b.number
}
case "number": return a.number - b.number
case "title": return a.title.localeCompare(b.title)
case "age": return new Date(b.createdAt).getTime() - new Date(a.createdAt).getTime()
case "age": return timestamps!.get(b.url)! - timestamps!.get(a.url)!
case "status": {
const sc = Number(a.isDraft) - Number(b.isDraft)
return sc !== 0 ? sc : a.repo.localeCompare(b.repo)
}
default: return 0
}
})
return prs
}, [allPRs, repoFilter, authorFilter, statusFilter, searchQuery, sortMode, detailsMap])
}, [filteredPRs, sortMode, urgencyMap])

// Grouped data computation
const groupedData = useMemo(() => {
if (groupMode === "none") return null

if (groupMode === "repo") {
return groupByRepo(filteredPRs)
return groupByRepo(sortedPRs)
} else if (groupMode === "stack") {
return groupByStack(filteredPRs)
return groupByStack(sortedPRs)
} else if (groupMode === "repo-stack") {
return groupByRepoAndStack(filteredPRs)
return groupByRepoAndStack(sortedPRs)
}
return null
}, [filteredPRs, groupMode])
}, [sortedPRs, groupMode])

useEffect(() => {
if (selectedIndex >= filteredPRs.length) {
setSelectedIndex(Math.max(0, filteredPRs.length - 1))
if (selectedIndex >= sortedPRs.length) {
setSelectedIndex(Math.max(0, sortedPRs.length - 1))
}
}, [filteredPRs.length, selectedIndex])
}, [sortedPRs.length, selectedIndex])

// Always fetch details - needed for lifecycle state detection and attention sort
const prevDetailUrlsRef = useRef("")

useEffect(() => {
if (filteredPRs.length === 0) return

const cache = cacheRef.current
const toFetch = filteredPRs.filter(pr => !cache.hasDetails(pr.url))

if (toFetch.length === 0) {
// All cached, just update the map

const urlFingerprint = filteredPRs.map(pr => pr.url).sort().join('\\n')

const buildMap = () => {
if (urlFingerprint === prevDetailUrlsRef.current && toFetch.length === 0) return
const map = new Map<string, PRDetails>()
for (const pr of filteredPRs) {
const d = cache.getDetails(pr.url)
if (d) map.set(pr.url, d)
}
prevDetailUrlsRef.current = urlFingerprint
setDetailsMap(map)
}

if (toFetch.length === 0) {
buildMap()
return
}

// Fetch missing details
Promise.all(
toFetch.map(async (pr) => {
try {
const details = await fetchPRDetails(pr.repo, pr.number)
cache.setDetails(pr.url, details)
} catch { /* skip on error */ }
})
).then(() => {
const map = new Map<string, PRDetails>()
for (const pr of filteredPRs) {
const d = cache.getDetails(pr.url)
if (d) map.set(pr.url, d)
batchFetchPRDetails(toFetch).then((fetchedMap) => {
for (const [url, details] of fetchedMap.entries()) {
cache.setDetails(url, details)
}
setDetailsMap(map)
buildMap()
}).catch(() => {
buildMap()
})
}, [filteredPRs])

const selectedPR = filteredPRs[selectedIndex] ?? null
const selectedPR = sortedPRs[selectedIndex] ?? null

// Panel state management (shared hook handles data fetching + caching + prefetching)
const panel = usePanel(selectedPR, filteredPRs, selectedIndex)
Expand Down Expand Up @@ -237,17 +270,20 @@ export function LsCommand({ author, repoFilter: initialRepoFilter }: LsCommandPr
if (selectedIndex < listHeight) return 0
return selectedIndex - listHeight + 1
}, [selectedIndex, listHeight])
const visiblePRs = filteredPRs.slice(scrollOffset, scrollOffset + listHeight)
const visiblePRs = sortedPRs.slice(scrollOffset, scrollOffset + listHeight)
const visibleSelectedIndex = selectedIndex - scrollOffset

const showFlash = useCallback((msg: string) => {
setFlash(msg)
setTimeout(() => setFlash(null), 2000)
}, [])

const scrollOffsetRef = useRef(scrollOffset)
scrollOffsetRef.current = scrollOffset

const handleSelect = useCallback((index: number) => {
setSelectedIndex(scrollOffset + index)
}, [scrollOffset])
setSelectedIndex(scrollOffsetRef.current + index)
}, [])

useKeyboard((key) => {
if (searchMode) {
Expand Down Expand Up @@ -453,7 +489,7 @@ export function LsCommand({ author, repoFilter: initialRepoFilter }: LsCommandPr
if (repoFilter && !initialRepoFilter) { setRepoFilter(null); return }
renderer.destroy()
} else if (key.name === "j" || key.name === "down") {
setSelectedIndex((i) => Math.min(filteredPRs.length - 1, i + 1))
setSelectedIndex((i) => Math.min(sortedPRs.length - 1, i + 1))
} else if (key.name === "k" || key.name === "up") {
setSelectedIndex((i) => Math.max(0, i - 1))
} else if (key.name === "enter" || key.name === "return") {
Expand Down Expand Up @@ -561,8 +597,10 @@ export function LsCommand({ author, repoFilter: initialRepoFilter }: LsCommandPr
}
})

const openCount = allPRs.filter((pr) => !pr.isDraft).length
const draftCount = allPRs.filter((pr) => pr.isDraft).length
const { openCount, draftCount } = useMemo(() => ({
openCount: allPRs.filter((pr) => !pr.isDraft).length,
draftCount: allPRs.filter((pr) => pr.isDraft).length,
}), [allPRs])

if (error) {
return (
Expand Down Expand Up @@ -595,7 +633,7 @@ export function LsCommand({ author, repoFilter: initialRepoFilter }: LsCommandPr
</box>
<box>
<text fg="#9aa5ce">
{filteredPRs.length} PRs sort: {SORT_LABELS[sortMode]} view: {density} group: {groupMode === "none" ? "None" : groupMode === "repo-stack" ? "Repo→Stack" : groupMode}
{sortedPRs.length} PRs sort: {SORT_LABELS[sortMode]} view: {density} group: {groupMode === "none" ? "None" : groupMode === "repo-stack" ? "Repo→Stack" : groupMode}
</text>
</box>
</box>
Expand Down Expand Up @@ -692,10 +730,10 @@ export function LsCommand({ author, repoFilter: initialRepoFilter }: LsCommandPr
groupedData={groupedData}
groupMode={groupMode}
/>
{filteredPRs.length > listHeight && (
{sortedPRs.length > listHeight && (
<box paddingX={1} height={1}>
<text fg="#6b7089">
{scrollOffset + 1}-{Math.min(scrollOffset + listHeight, filteredPRs.length)} of {filteredPRs.length}
{scrollOffset + 1}-{Math.min(scrollOffset + listHeight, sortedPRs.length)} of {sortedPRs.length}
</text>
</box>
)}
Expand Down
Loading
Loading