diff --git a/docs/gh-work-item-drawer-cache.md b/docs/gh-work-item-drawer-cache.md new file mode 100644 index 000000000..8e95a48a6 --- /dev/null +++ b/docs/gh-work-item-drawer-cache.md @@ -0,0 +1,118 @@ +# GitHub Work Item Drawer: Cache & Latency + +## Problem + +`GitHubItemDialog` clears `details` to `null` on every open and waits for `gh:workItemDetails` before rendering. Reopening the same item therefore pays full IPC + `gh` process startup latency again. + +The slow path is mostly process overhead and fan-out in `src/main/github/work-item-details.ts`, not GitHub API quota. + +## What is true today + +- `gh api --cache` reduces network/API pressure, but it does not remove local process startup, JSON parsing, or IPC serialization. +- A 304 response does not consume primary REST rate-limit quota, but it is still a network round-trip. With `--cache`, many calls are served from local cache anyway. +- The issue details path is currently multi-call: +1. `getWorkItem` (`/issues/:n` or PR fallback). +2. `getIssueBodyAndComments` (`/issues/:n` and `/issues/:n/comments`). +3. `getWorkItemParticipants` (GraphQL participants). +4. `getMentionParticipants` (GraphQL user hydration for visible authors). + +So “3 spawns” understates the real hot path. + +## Goals + +- Reopen latency should be near-instant for recently viewed items. +- Cold issue-open latency should drop by reducing `gh` command count. +- Correctness must survive local mutations, repo/source switches, and multi-window usage. + +## Non-goals + +- No disk-persistent cache in this iteration. +- No attempt to replace PR files/diff path with GraphQL. + +## Design + +### 1. Renderer SWR cache with paint-first reads + +Add a module-level LRU cache in `GitHubItemDialog.tsx` keyed by: + +`repoPath + issueSourcePreference + type + number` + +Cache value: + +- `details` +- `fetchedAt` +- `pending?: Promise` +- `error?: string` + +Behavior: + +1. On open, render cached `details` immediately when present (do not clear to `null`). +2. If entry age <= `FRESH_MS` (30s), skip fetch. +3. If stale or missing, fetch in background and replace cache + UI when resolved. +4. On fetch failure with cached data, keep stale data visible and show non-blocking error state. +5. On fetch failure without cached data, show blocking error state. + +### 2. In-flight dedupe in renderer + +Store a single pending promise per key in the same cache entry. Concurrent opens/re-renders for the same key must await the same promise. + +This dedupe must be keyed the same as the data cache key to avoid cross-repo or cross-source collisions. + +### 3. Explicit invalidation rules + +“Background refetch is authoritative” is necessary but not sufficient. + +Invalidate (or patch + mark stale) on successful local mutations: + +- issue state/labels/assignees/body edits +- new comments/reactions +- PR review comment create/resolve + +Scope: + +- per-item key in current window +- broadcast to other windows via main-process event (`gh:workItemMutated`) so their caches invalidate too + +Also invalidate on context switches: + +- `repoPath` change +- issue source preference change (`origin`/`upstream`/`auto`) +- sign-out/account change + +For out-of-band mutations (web UI/other tools), rely on TTL + manual refresh action in drawer header. + +### 4. Main-process issue fetch collapse (GraphQL-first) + +Do not claim “one call returns everything” unless we actually ship and verify it. + +Feasible single GraphQL issue query fields: + +- issue body +- labels +- assignees +- participants +- comments(first: N) with author login + avatarUrl + body + createdAt + url + +Limits and required fallbacks: + +- GraphQL pagination still applies (`first: 100`). More comments require paging. +- Some comment authors can be null/ghost; renderer must keep existing fallback behavior. +- If GraphQL fails (permissions, partial errors), fall back to current REST+GraphQL path. +- Keep `getMentionParticipants` only if query omits non-participant visible authors; otherwise remove it. + +PR path remains unchanged in this doc. PR file/diff/check behavior is intentionally out of scope. + +## Edge cases this design must handle + +- Reopen same item after optimistic comment: optimistic comment must survive stale cache reads until authoritative fetch includes it. +- Switching between upstream/origin issue source with same issue number must never reuse the wrong cache entry. +- Item-type collision (`issue #123` vs `pr #123`) must never reuse cache entry. +- Drawer close/open races: stale request responses must still be dropped (`requestIdRef` guard stays). +- Unauthorized/404 should not overwrite valid cached data with empty shells. + +## Rollout + +1. Implement renderer SWR + in-flight dedupe + stale-on-error behavior. +2. Add mutation-driven invalidation and cross-window invalidation event. +3. Implement GraphQL-first issue details with strict fallback. +4. Keep telemetry: measure open-to-first-paint and open-to-fresh-data before/after. diff --git a/src/main/github/work-item-details.test.ts b/src/main/github/work-item-details.test.ts index 054eeb2df..b6f92b741 100644 --- a/src/main/github/work-item-details.test.ts +++ b/src/main/github/work-item-details.test.ts @@ -49,7 +49,7 @@ describe('getWorkItemDetails', () => { acquireMock.mockResolvedValue(undefined) }) - it('passes row type into the lookup and fetches issue comments from the issue source', async () => { + it('uses the collapsed GraphQL issue query as the hot path', async () => { getWorkItemMock.mockResolvedValueOnce({ id: 'issue:923', type: 'issue', @@ -61,21 +61,84 @@ describe('getWorkItemDetails', () => { updatedAt: '2026-04-01T00:00:00Z', author: 'octocat' }) - getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'stablyai', repo: 'orca' }) - ghExecFileAsyncMock - .mockResolvedValueOnce({ stdout: JSON.stringify({ body: 'Issue body' }) }) - .mockResolvedValueOnce({ stdout: '[]' }) + getIssueOwnerRepoMock.mockResolvedValue({ owner: 'stablyai', repo: 'orca' }) + ghExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify({ + data: { + repository: { + issue: { + body: 'Issue body', + assignees: { nodes: [{ login: 'jinjing' }] }, + participants: { + nodes: [{ login: 'octocat', avatarUrl: 'https://x/y', name: 'Octo Cat' }] + }, + comments: { + nodes: [ + { + databaseId: 7, + body: 'first', + createdAt: '2026-04-01T00:00:00Z', + url: 'https://github.com/stablyai/orca/issues/923#issuecomment-7', + author: { login: 'octocat', avatarUrl: 'https://x/y' } + } + ] + } + } + } + } + }) + }) const details = await getWorkItemDetails('/repo-root', 923, 'issue') expect(getWorkItemMock).toHaveBeenCalledWith('/repo-root', 923, 'issue') + // Why: a single gh subprocess call replaces the previous REST + REST + GraphQL fan-out. + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) + expect(ghExecFileAsyncMock.mock.calls[0][0][0]).toBe('api') + expect(ghExecFileAsyncMock.mock.calls[0][0][1]).toBe('graphql') + expect(details?.body).toBe('Issue body') + expect(details?.assignees).toEqual(['jinjing']) + expect(details?.comments).toHaveLength(1) + expect(details?.comments[0].id).toBe(7) + expect(details?.participants?.[0]?.login).toBe('octocat') + }) + + it('falls back to REST + GraphQL when the collapsed issue query fails', async () => { + getWorkItemMock.mockResolvedValueOnce({ + id: 'issue:923', + type: 'issue', + number: 923, + title: 'Use upstream issues', + state: 'open', + url: 'https://github.com/stablyai/orca/issues/923', + labels: [], + updatedAt: '2026-04-01T00:00:00Z', + author: 'octocat' + }) + getIssueOwnerRepoMock.mockResolvedValue({ owner: 'stablyai', repo: 'orca' }) + // Collapsed GraphQL throws → fallback path picks up. + ghExecFileAsyncMock + .mockRejectedValueOnce(new Error('GraphQL error')) + .mockResolvedValueOnce({ stdout: JSON.stringify({ body: 'Issue body' }) }) + .mockResolvedValueOnce({ stdout: '[]' }) + .mockResolvedValueOnce({ + stdout: JSON.stringify({ + data: { repository: { issue: { participants: { nodes: [] } } } } + }) + }) + .mockResolvedValueOnce({ + stdout: JSON.stringify({ data: {} }) + }) + + const details = await getWorkItemDetails('/repo-root', 923, 'issue') + expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( - 1, + 2, ['api', '--cache', '60s', 'repos/stablyai/orca/issues/923'], { cwd: '/repo-root' } ) expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( - 2, + 3, ['api', '--cache', '60s', 'repos/stablyai/orca/issues/923/comments?per_page=100'], { cwd: '/repo-root' } ) diff --git a/src/main/github/work-item-details.ts b/src/main/github/work-item-details.ts index c0dbc1b46..a0ea1983d 100644 --- a/src/main/github/work-item-details.ts +++ b/src/main/github/work-item-details.ts @@ -32,6 +32,135 @@ const WORK_ITEM_PARTICIPANTS_QUERY = `query($owner: String!, $repo: String!, $nu } }` +// Why: a single GraphQL round-trip replaces three serial gh subprocesses on +// the issue path (REST issue + REST comments + GraphQL participants). The +// previous fan-out could spawn ~3 `gh` processes per drawer-open; this drops +// it to one. We still fall back to the legacy REST+GraphQL path if the +// collapsed query throws or returns missing data — see the strict-fallback +// branch in getWorkItemDetails. +const ISSUE_DETAILS_QUERY = `query($owner: String!, $repo: String!, $number: Int!) { + repository(owner: $owner, name: $repo) { + issue(number: $number) { + body + assignees(first: 50) { nodes { login } } + participants(first: 100) { + nodes { login avatarUrl(size: 48) ... on User { name } } + } + comments(first: 100) { + nodes { + databaseId + body + createdAt + url + author { + login + avatarUrl(size: 48) + ... on Bot { __typename } + } + } + } + } + } +}` + +type GraphQLIssueDetailsResponse = { + data?: { + repository?: { + issue?: { + body?: string | null + assignees?: { nodes?: { login?: string }[] } + participants?: { nodes?: GitHubAssignableUser[] } + comments?: { + nodes?: { + databaseId?: number | null + body?: string | null + createdAt?: string | null + url?: string | null + author?: { + login?: string | null + avatarUrl?: string | null + __typename?: string + } | null + }[] + } + } | null + } | null + } + errors?: { message?: string }[] +} + +async function getIssueDetailsViaGraphQL( + repoPath: string, + issueNumber: number +): Promise<{ + body: string + comments: PRComment[] + assignees: string[] + participants: GitHubAssignableUser[] +} | null> { + const ownerRepo = await getIssueOwnerRepo(repoPath) + if (!ownerRepo) { + return null + } + try { + const { stdout } = await ghExecFileAsync( + [ + 'api', + 'graphql', + '-f', + `query=${ISSUE_DETAILS_QUERY}`, + '-f', + `owner=${ownerRepo.owner}`, + '-f', + `repo=${ownerRepo.repo}`, + '-F', + `number=${issueNumber}` + ], + { cwd: repoPath } + ) + const parsed = JSON.parse(stdout) as GraphQLIssueDetailsResponse + if (parsed.errors && parsed.errors.length > 0) { + // Why: any partial GraphQL error (permissions, unknown field on a fork) + // forces the strict REST fallback so the drawer never paints a half-built + // shell. The fallback path's behavior is the historical contract. + return null + } + const issue = parsed.data?.repository?.issue + if (!issue) { + return null + } + const comments: PRComment[] = (issue.comments?.nodes ?? []) + .filter((c) => typeof c.databaseId === 'number') + .map((c) => ({ + id: c.databaseId as number, + author: c.author?.login ?? 'ghost', + authorAvatarUrl: c.author?.avatarUrl ?? '', + body: c.body ?? '', + createdAt: c.createdAt ?? '', + url: c.url ?? '', + isBot: c.author?.__typename === 'Bot' + })) + const assignees = (issue.assignees?.nodes ?? []) + .map((a) => a.login) + .filter((login): login is string => Boolean(login)) + const participants: GitHubAssignableUser[] = (issue.participants?.nodes ?? []) + .filter((u) => Boolean(u.login)) + .map((u) => ({ + login: u.login, + name: u.name ?? null, + avatarUrl: u.avatarUrl ?? '' + })) + return { + body: issue.body ?? '', + comments, + assignees, + participants + } + } catch { + return null + } +} + function mergeGitHubUsers(users: GitHubAssignableUser[]): GitHubAssignableUser[] { const byLogin = new Map() for (const user of users) { @@ -404,8 +533,25 @@ export async function getWorkItemDetails( await acquire() try { if (item.type === 'issue') { - // Why: fetch body/comments and GraphQL participants in parallel; the - // mention-participant merge is a cheap local operation afterward. + // Why: try the collapsed single-GraphQL path first — body, assignees, + // participants, and comments all return in one round-trip. On any + // failure (permissions, partial errors, non-GitHub remote), strictly + // fall back to the legacy REST+GraphQL fan-out so historical behavior + // is preserved. The GraphQL `participants` connection includes every + // commenter, so we skip the extra `getMentionParticipants` aliased + // user-hydration trip when the collapsed path succeeds. + const collapsed = await getIssueDetailsViaGraphQL(repoPath, item.number) + if (collapsed) { + return { + item, + body: collapsed.body, + comments: collapsed.comments, + assignees: collapsed.assignees, + participants: collapsed.participants + } + } + // Why: fall back to body/comments and GraphQL participants in parallel; + // the mention-participant merge is a cheap local operation afterward. const [{ body, comments, assignees }, participants] = await Promise.all([ getIssueBodyAndComments(repoPath, item.number), getWorkItemParticipants(repoPath, item) diff --git a/src/main/ipc/github.ts b/src/main/ipc/github.ts index 8fa182e31..4b3e9c202 100644 --- a/src/main/ipc/github.ts +++ b/src/main/ipc/github.ts @@ -2,7 +2,7 @@ the repo-path validation, preference-threading, and stats wiring patterns are reviewable as one surface. Splitting by feature area would risk drifting validation/gate conventions across handler files. */ -import { ipcMain } from 'electron' +import { ipcMain, webContents } from 'electron' import { resolve } from 'path' import type { Repo, GitHubIssueUpdate } from '../../shared/types' import type { Store } from '../persistence' @@ -73,6 +73,31 @@ import type { UpdatePullRequestBySlugArgs } from '../../shared/github-project-types' +// Why: notify every renderer (each window has its own SWR cache instance) +// that a work item was mutated locally so they can drop their cached entry +// and refetch on the next open. Only emitted after a successful mutation. +// We skip the originating webContents because that renderer already updated +// its cache optimistically — re-broadcasting would race the optimistic write +// and erase it. +function broadcastWorkItemMutated( + payload: { + repoPath: string + type: 'issue' | 'pr' + number: number + }, + senderId?: number +): void { + for (const wc of webContents.getAllWebContents()) { + if (wc.isDestroyed()) { + continue + } + if (senderId !== undefined && wc.id === senderId) { + continue + } + wc.send('gh:workItemMutated', payload) + } +} + // Why: returns the full Repo object instead of just the path string so that // callers have access to repo.id for stat tracking and other context. function assertRegisteredRepo(repoPath: string, store: Store): Repo { @@ -231,16 +256,21 @@ export function registerGitHubHandlers(store: Store, stats: StatsCollector): voi ipcMain.handle( 'gh:resolveReviewThread', - (_event, args: { repoPath: string; threadId: string; resolve: boolean }) => { + async (_event, args: { repoPath: string; threadId: string; resolve: boolean }) => { const repo = assertRegisteredRepo(args.repoPath, store) + // Why: thread resolve doesn't carry the PR number, so we cannot target + // a specific cache entry. The renderer cache stores per-(repo, type, number) + // entries — emitting a path-wide invalidation here would require a new + // event shape; instead, the drawer's existing thread-resolve UI updates + // its local state immediately and the next reopen pays one fresh fetch. return resolveReviewThread(repo.path, args.threadId, args.resolve) } ) ipcMain.handle( 'gh:addPRReviewCommentReply', - ( - _event, + async ( + event, args: { repoPath: string prNumber: number @@ -269,7 +299,7 @@ export function registerGitHubHandlers(store: Store, stats: StatsCollector): voi if (!args.body?.trim()) { return { ok: false, error: 'Comment body required' } } - return addPRReviewCommentReply( + const result = await addPRReviewCommentReply( repo.path, args.prNumber, args.commentId, @@ -278,13 +308,20 @@ export function registerGitHubHandlers(store: Store, stats: StatsCollector): voi args.path, args.line ) + if (result.ok) { + broadcastWorkItemMutated( + { repoPath: repo.path, type: 'pr', number: args.prNumber }, + event.sender.id + ) + } + return result } ) ipcMain.handle( 'gh:addPRReviewComment', - ( - _event, + async ( + event, args: { repoPath: string prNumber: number @@ -324,7 +361,7 @@ export function registerGitHubHandlers(store: Store, stats: StatsCollector): voi if (!args.body?.trim()) { return { ok: false, error: 'Comment body required' } } - return addPRReviewComment({ + const result = await addPRReviewComment({ repoPath: repo.path, prNumber: args.prNumber, commitId: args.commitId.trim(), @@ -333,31 +370,52 @@ export function registerGitHubHandlers(store: Store, stats: StatsCollector): voi startLine: args.startLine, body: args.body.trim() }) + if (result.ok) { + broadcastWorkItemMutated( + { repoPath: repo.path, type: 'pr', number: args.prNumber }, + event.sender.id + ) + } + return result } ) ipcMain.handle( 'gh:updatePRTitle', - (_event, args: { repoPath: string; prNumber: number; title: string }) => { + async (event, args: { repoPath: string; prNumber: number; title: string }) => { const repo = assertRegisteredRepo(args.repoPath, store) - return updatePRTitle(repo.path, args.prNumber, args.title) + const ok = await updatePRTitle(repo.path, args.prNumber, args.title) + if (ok) { + broadcastWorkItemMutated( + { repoPath: repo.path, type: 'pr', number: args.prNumber }, + event.sender.id + ) + } + return ok } ) ipcMain.handle( 'gh:mergePR', - ( - _event, + async ( + event, args: { repoPath: string; prNumber: number; method?: 'merge' | 'squash' | 'rebase' } ) => { const repo = assertRegisteredRepo(args.repoPath, store) - return mergePR(repo.path, args.prNumber, args.method) + const result = await mergePR(repo.path, args.prNumber, args.method) + if (result.ok) { + broadcastWorkItemMutated( + { repoPath: repo.path, type: 'pr', number: args.prNumber }, + event.sender.id + ) + } + return result } ) ipcMain.handle( 'gh:updateIssue', - (_event, args: { repoPath: string; number: number; updates: GitHubIssueUpdate }) => { + async (event, args: { repoPath: string; number: number; updates: GitHubIssueUpdate }) => { const repo = assertRegisteredRepo(args.repoPath, store) if (typeof args.number !== 'number' || !Number.isInteger(args.number) || args.number < 1) { return { ok: false, error: 'Invalid issue number' } @@ -365,13 +423,23 @@ export function registerGitHubHandlers(store: Store, stats: StatsCollector): voi if (!args.updates || typeof args.updates !== 'object') { return { ok: false, error: 'Updates object is required' } } - return updateIssue(repo.path, args.number, args.updates) + const result = await updateIssue(repo.path, args.number, args.updates) + if (result.ok) { + broadcastWorkItemMutated( + { repoPath: repo.path, type: 'issue', number: args.number }, + event.sender.id + ) + } + return result } ) ipcMain.handle( 'gh:addIssueComment', - (_event, args: { repoPath: string; number: number; body: string }) => { + async ( + event, + args: { repoPath: string; number: number; body: string; type?: 'issue' | 'pr' } + ) => { const repo = assertRegisteredRepo(args.repoPath, store) if (typeof args.number !== 'number' || !Number.isInteger(args.number) || args.number < 1) { return { ok: false, error: 'Invalid issue number' } @@ -379,7 +447,19 @@ export function registerGitHubHandlers(store: Store, stats: StatsCollector): voi if (!args.body?.trim()) { return { ok: false, error: 'Comment body required' } } - return addIssueComment(repo.path, args.number, args.body.trim()) + const result = await addIssueComment(repo.path, args.number, args.body.trim()) + if (result.ok) { + // Why: PR conversation comments hit `/issues/N/comments` too, but the + // drawer's cache key uses type='pr'. The caller passes through which + // drawer they're posting from so we only invalidate the matching key + // — broadcasting both would evict an unrelated PR/issue that happens + // to share the number. + broadcastWorkItemMutated( + { repoPath: repo.path, type: args.type ?? 'issue', number: args.number }, + event.sender.id + ) + } + return result } ) diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index bcb0c2143..e4d3fa2af 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -597,6 +597,12 @@ export type PreloadApi = { repoPath: string number: number body: string + /** Why: GitHub stores PR conversation comments under `/issues/N/comments` + * too, so the IPC and `gh` call paths are identical. The renderer cache + * key is keyed by the drawer's `type`, so callers pass it through to + * scope the cross-window invalidation broadcast correctly and avoid + * evicting an unrelated PR/issue that happens to share the number. */ + type?: 'issue' | 'pr' }) => Promise addPRReviewCommentReply: (args: { repoPath: string @@ -610,6 +616,14 @@ export type PreloadApi = { addPRReviewComment: (args: GitHubPRReviewCommentInput) => Promise listLabels: (args: { repoPath: string }) => Promise listAssignableUsers: (args: { repoPath: string }) => Promise + /** + * Subscribe to local-mutation broadcasts. Used by the work-item-drawer + * cache to invalidate entries across windows after a successful mutation. + * Returns an unsubscribe function. + */ + onWorkItemMutated: ( + callback: (payload: { repoPath: string; type: 'issue' | 'pr'; number: number }) => void + ) => () => void checkOrcaStarred: () => Promise starOrca: () => Promise /** diff --git a/src/preload/index.ts b/src/preload/index.ts index a72438a29..c287ba204 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -677,6 +677,7 @@ const api = { repoPath: string number: number body: string + type?: 'issue' | 'pr' }): Promise => ipcRenderer.invoke('gh:addIssueComment', args), addPRReviewCommentReply: (args: { @@ -705,6 +706,21 @@ const api = { listAssignableUsers: (args: { repoPath: string }): Promise => ipcRenderer.invoke('gh:listAssignableUsers', args), + // Why: every renderer subscribes to local mutation broadcasts so each + // window's work-item-details cache invalidates the affected entry. The + // event fires after a successful mutation in any window — see + // src/main/ipc/github.ts broadcastWorkItemMutated. + onWorkItemMutated: ( + callback: (payload: { repoPath: string; type: 'issue' | 'pr'; number: number }) => void + ): (() => void) => { + const listener = ( + _event: Electron.IpcRendererEvent, + payload: { repoPath: string; type: 'issue' | 'pr'; number: number } + ): void => callback(payload) + ipcRenderer.on('gh:workItemMutated', listener) + return () => ipcRenderer.removeListener('gh:workItemMutated', listener) + }, + checkOrcaStarred: (): Promise => ipcRenderer.invoke('gh:checkOrcaStarred'), starOrca: (): Promise => ipcRenderer.invoke('gh:starOrca'), diff --git a/src/renderer/src/components/GitHubItemDialog.tsx b/src/renderer/src/components/GitHubItemDialog.tsx index 1c04c23ab..63bde0578 100644 --- a/src/renderer/src/components/GitHubItemDialog.tsx +++ b/src/renderer/src/components/GitHubItemDialog.tsx @@ -492,6 +492,101 @@ function PRDiffTreeView({ ) } +// Why: SWR cache for the work-item details fetch. Reopening the same drawer +// pays full IPC + `gh` process startup latency without this; with it, cached +// data paints immediately while a background refetch keeps the view honest. +// Cache is keyed by repoPath + issueSourcePreference + type + number so +// upstream/origin source toggles and issue#N vs pr#N never collide. Bounded +// to ~50 entries to cap memory; entries older than FRESH_MS trigger a +// background refetch on open. See docs/gh-work-item-drawer-cache.md. +const WORK_ITEM_DETAILS_CACHE_MAX = 50 +const WORK_ITEM_DETAILS_FRESH_MS = 30_000 +type WorkItemDetailsCacheEntry = { + details: GitHubWorkItemDetails | null + fetchedAt: number + pending?: Promise + error?: string +} +const workItemDetailsCache = new Map() + +function getWorkItemDetailsCacheKey(args: { + repoPath: string + issueSourcePreference: string | undefined + type: 'issue' | 'pr' + number: number +}): string { + // Why: include all axes that change which (repo, item) the IPC resolves to. + // `\0` separator avoids ambiguity between fields that may contain `:` or `/`. + return [args.repoPath, args.issueSourcePreference ?? 'auto', args.type, args.number].join('\0') +} + +function touchWorkItemDetailsCache(key: string, entry: WorkItemDetailsCacheEntry): void { + // Why: re-insert to move to MRU position; Map preserves insertion order so + // the oldest key is always first when evicting. + workItemDetailsCache.delete(key) + workItemDetailsCache.set(key, entry) + while (workItemDetailsCache.size > WORK_ITEM_DETAILS_CACHE_MAX) { + const oldest = workItemDetailsCache.keys().next().value + if (oldest === undefined) { + break + } + workItemDetailsCache.delete(oldest) + } +} + +// Why: exposed so mutation handlers (in this file and elsewhere) can drop a +// stale entry after a successful local mutation. Cross-window invalidation +// arrives via the `gh:workItemMutated` event listener installed below. +export function invalidateWorkItemDetailsCacheForKey(key: string): void { + workItemDetailsCache.delete(key) +} + +// Why: monotonically increases on every invalidation so an in-flight refetch +// that started before a mutation can detect that its result is stale and +// must not be written back. Without this, a mutation that lands while a +// refetch is in flight would have its invalidation silently undone when the +// stale promise resolves and re-populates the entry. +let workItemDetailsCacheGeneration = 0 + +// Why: when we don't have the exact cache key (e.g. an event from another +// window only carries repoPath + number + type), drop every entry that +// matches the (repoPath, type, number) tuple regardless of source preference. +function invalidateWorkItemDetailsCacheByMatch(args: { + repoPath: string + type: 'issue' | 'pr' + number: number +}): void { + workItemDetailsCacheGeneration += 1 + const suffix = `\0${args.type}\0${args.number}` + const prefix = `${args.repoPath}\0` + for (const key of Array.from(workItemDetailsCache.keys())) { + if (key.startsWith(prefix) && key.endsWith(suffix)) { + workItemDetailsCache.delete(key) + } + } +} + +// Why: install once at module load — every dialog instance shares the cache, +// so a single subscription is enough. The preload bridge re-emits the +// main-process broadcast for every window, so each renderer invalidates its +// own cache when any window's mutation lands. We track the unsubscribe so +// Vite HMR doesn't accumulate listeners across module reloads in dev. +let workItemMutatedUnsub: (() => void) | undefined +if (typeof window !== 'undefined' && window.api?.gh?.onWorkItemMutated) { + workItemMutatedUnsub = window.api.gh.onWorkItemMutated((payload) => { + invalidateWorkItemDetailsCacheByMatch({ + repoPath: payload.repoPath, + type: payload.type, + number: payload.number + }) + }) +} +if (typeof import.meta !== 'undefined' && import.meta.hot) { + import.meta.hot.dispose(() => { + workItemMutatedUnsub?.() + }) +} + // Why: bounded LRU — opening many PRs with many files during a session // would otherwise grow this module-level map without bound until reload. const PR_FILE_CONTENT_CACHE_MAX = 64 @@ -1088,7 +1183,8 @@ function ConversationTab({ : await window.api.gh.addIssueComment({ repoPath, number: item.number, - body: `@${comment.author} ${replyBody}` + body: `@${comment.author} ${replyBody}`, + type: item.type }) if (!result.ok) { @@ -1351,6 +1447,7 @@ function ConversationTab({ className="mt-1" repoPath={repoPath} issueNumber={item.number} + itemType={item.type} mentionOptions={mentionOptions} onCommentAdded={onCommentAdded} /> @@ -1698,6 +1795,7 @@ function GHEditSection({ localLabels, onStateChange, onLabelsChange, + onMutated, assignees, onUse }: { @@ -1708,6 +1806,10 @@ function GHEditSection({ localLabels: string[] onStateChange: (state: GitHubWorkItem['state']) => void onLabelsChange: (labels: string[]) => void + /** Why: called after a successful issue mutation so the parent dialog can + * invalidate its work-item-details cache entry. Without this, reopening the + * drawer in the FRESH_MS window would paint pre-mutation data. */ + onMutated: () => void assignees: string[] onUse: (item: GitHubWorkItem) => void }): React.JSX.Element | null { @@ -1788,6 +1890,7 @@ function GHEditSection({ onSuccess: () => { patchWorkItem(item.id, { state: newState }) patchProjectRowIfNeeded({ state: newState }) + onMutated() }, onError: (err) => toast.error(err) }) @@ -1801,7 +1904,8 @@ function GHEditSection({ patchWorkItem, patchProjectRowIfNeeded, run, - onStateChange + onStateChange, + onMutated ] ) @@ -1825,7 +1929,9 @@ function GHEditSection({ patchWorkItem(item.id, { labels: newLabels }) patchProjectRowIfNeeded({ labels: newLabels }) }, - onSuccess: () => {}, + onSuccess: () => { + onMutated() + }, onRevert: () => { onLabelsChange(prevLabels) patchWorkItem(item.id, { labels: prevLabels }) @@ -1852,7 +1958,9 @@ function GHEditSection({ patchWorkItem(item.id, { labels: prevLabels }) patchProjectRowIfNeeded({ labels: prevLabels }) }, - onSuccess: () => {}, + onSuccess: () => { + onMutated() + }, onError: (err) => toast.error(err) }) } @@ -1866,7 +1974,8 @@ function GHEditSection({ patchWorkItem, patchProjectRowIfNeeded, run, - onLabelsChange + onLabelsChange, + onMutated ] ) @@ -1896,7 +2005,9 @@ function GHEditSection({ setLocalAssignees(prevAssignees) patchProjectRowIfNeeded({ assignees: prevAssignees }) }, - onSuccess: () => {}, + onSuccess: () => { + onMutated() + }, onError: (err) => toast.error(err) }) } else { @@ -1912,7 +2023,9 @@ function GHEditSection({ setLocalAssignees(newAssignees) patchProjectRowIfNeeded({ assignees: newAssignees }) }, - onSuccess: () => {}, + onSuccess: () => { + onMutated() + }, onRevert: () => { setLocalAssignees(prevAssignees) patchProjectRowIfNeeded({ assignees: prevAssignees }) @@ -1921,7 +2034,7 @@ function GHEditSection({ }) } }, - [item.number, repoPath, projectOrigin, localAssignees, patchProjectRowIfNeeded, run] + [item.number, repoPath, projectOrigin, localAssignees, patchProjectRowIfNeeded, run, onMutated] ) if (item.type === 'pr') { @@ -2118,12 +2231,14 @@ function GHCommentComposer({ className, repoPath, issueNumber, + itemType, mentionOptions, onCommentAdded }: { className?: string repoPath: string issueNumber: number + itemType: 'issue' | 'pr' mentionOptions: MentionOption[] onCommentAdded: (comment: PRComment) => void }): React.JSX.Element { @@ -2150,7 +2265,8 @@ function GHCommentComposer({ const result = await window.api.gh.addIssueComment({ repoPath, number: issueNumber, - body: trimmed + body: trimmed, + type: itemType }) if (result.ok) { setBody('') @@ -2291,6 +2407,29 @@ export default function GitHubItemDialog({ const workItemState = workItem?.state const workItemLabels = workItem?.labels + // Why: the cache key has to include the issue source preference so a user + // toggling between origin/upstream for the same issue number doesn't read + // back the wrong repo's details. We pull it from the repos slice rather + // than threading it as a prop because every existing call site already has + // the repo registered in the store. + const issueSourcePreference = useAppStore((s) => { + if (!repoPath) { + return undefined + } + return s.repos.find((r) => r.path === repoPath)?.issueSourcePreference + }) + const detailsCacheKey = useMemo(() => { + if (!workItem || !repoPath) { + return null + } + return getWorkItemDetailsCacheKey({ + repoPath, + issueSourcePreference, + type: workItem.type, + number: workItem.number + }) + }, [repoPath, workItem, issueSourcePreference]) + // Why: reset lifted edit state when the dialog switches items or when the // same item receives an optimistic cache patch from the surrounding table. useEffect(() => { @@ -2339,7 +2478,7 @@ export default function GitHubItemDialog({ }, [workItem]) useEffect(() => { - if (!workItem || !repoPath) { + if (!workItem || !repoPath || !detailsCacheKey) { setDetails(null) setError(null) return @@ -2358,34 +2497,124 @@ export default function GitHubItemDialog({ optimisticCommentsRef.current = [] } prevItemIdRef.current = workItem.id - setLoading(true) - setError(null) - setDetails(null) setTab('conversation') - window.api.gh - .workItemDetails({ repoPath, number: workItem.number, type: workItem.type }) + const cached = workItemDetailsCache.get(detailsCacheKey) + const now = Date.now() + const hasFreshData = cached?.details && now - cached.fetchedAt <= WORK_ITEM_DETAILS_FRESH_MS + + // Why: paint cached data immediately when we have it so reopen feels + // instant. Only fall back to a blocking spinner when there's nothing to + // show. Optimistic comments still merge below for both paths. + if (cached?.details) { + const opt = optimisticCommentsRef.current + let painted = cached.details + if (opt.length > 0) { + const ids = new Set(painted.comments.map((c) => c.id)) + const missing = opt.filter((c) => !ids.has(c.id)) + if (missing.length > 0) { + painted = { ...painted, comments: [...painted.comments, ...missing] } + } + } + setDetails(painted) + setError(null) + setLoading(false) + } else { + setDetails(null) + setError(null) + setLoading(true) + } + + if (hasFreshData) { + return + } + + // Why: dedupe concurrent opens for the same key — concurrent dialogs or + // a rapid close→reopen must share one in-flight promise instead of + // racing two `gh` subprocesses against each other. + const inflight: Promise = + cached?.pending ?? + window.api.gh.workItemDetails({ + repoPath, + number: workItem.number, + type: workItem.type + }) + + // Why: snapshot the invalidation generation at fetch start; if the + // generation advances before we resolve, a mutation invalidated the + // entry mid-flight and we must not write a stale result back. + const launchedAtGeneration = workItemDetailsCacheGeneration + + if (!cached?.pending) { + touchWorkItemDetailsCache(detailsCacheKey, { + details: cached?.details ?? null, + fetchedAt: cached?.fetchedAt ?? 0, + pending: inflight, + error: cached?.error + }) + } + + inflight .then((result) => { + const invalidatedMidFlight = workItemDetailsCacheGeneration !== launchedAtGeneration + // Why: 404/unauthorized must not overwrite valid cached data. When the + // IPC resolves to null and we already have cached details, keep the + // stale data — only blank entries get the null payload. + const prev = workItemDetailsCache.get(detailsCacheKey) + if (invalidatedMidFlight) { + // Skip cache write entirely — the entry was deliberately dropped. + } else if (result === null && prev?.details) { + touchWorkItemDetailsCache(detailsCacheKey, { + details: prev.details, + fetchedAt: prev.fetchedAt, + error: undefined + }) + } else { + touchWorkItemDetailsCache(detailsCacheKey, { + details: result, + fetchedAt: Date.now(), + error: undefined + }) + } if (requestId !== requestIdRef.current) { return } // Why: merge any comments the user posted optimistically while the // detail fetch was in-flight, using id to avoid duplicates. const opt = optimisticCommentsRef.current - if (opt.length > 0 && result) { - const fetchedIds = new Set(result.comments.map((c: PRComment) => c.id)) + let merged = result + if (opt.length > 0 && merged) { + const fetchedIds = new Set(merged.comments.map((c: PRComment) => c.id)) const missing = opt.filter((c) => !fetchedIds.has(c.id)) if (missing.length > 0) { - result = { ...result, comments: [...result.comments, ...missing] } + merged = { ...merged, comments: [...merged.comments, ...missing] } } } - setDetails(result) + if (merged !== null || !cached?.details) { + setDetails(merged) + } }) .catch((err) => { + const message = err instanceof Error ? err.message : 'Failed to load details' + const invalidatedMidFlight = workItemDetailsCacheGeneration !== launchedAtGeneration + const prev = workItemDetailsCache.get(detailsCacheKey) + // Why: stale-on-error — keep cached data if we have it, drop the + // pending promise so the next open can retry. Only surface the + // blocking error when nothing is cached. If invalidated mid-flight, + // don't restore stale data into the now-empty entry. + if (!invalidatedMidFlight) { + touchWorkItemDetailsCache(detailsCacheKey, { + details: prev?.details ?? null, + fetchedAt: prev?.fetchedAt ?? 0, + error: message + }) + } if (requestId !== requestIdRef.current) { return } - setError(err instanceof Error ? err.message : 'Failed to load details') + if (!prev?.details) { + setError(message) + } }) .finally(() => { if (requestId !== requestIdRef.current) { @@ -2393,7 +2622,7 @@ export default function GitHubItemDialog({ } setLoading(false) }) - }, [repoPath, workItem]) + }, [repoPath, workItem, detailsCacheKey]) const Icon = workItem?.type === 'pr' ? GitPullRequest : CircleDot const body = details?.body ?? '' @@ -2422,8 +2651,25 @@ export default function GitHubItemDialog({ comments: [comment] } }) + // Why: keep the module-level cache in sync so a reopen paints the new + // comment without waiting on a refetch. Mark fetchedAt as stale (0) so + // the next open still triggers a background refresh to pick up + // server-side fields like reaction groups or thread bindings. + if (detailsCacheKey) { + const prev = workItemDetailsCache.get(detailsCacheKey) + if (prev?.details) { + const ids = new Set(prev.details.comments.map((c) => c.id)) + if (!ids.has(comment.id)) { + touchWorkItemDetailsCache(detailsCacheKey, { + details: { ...prev.details, comments: [...prev.details.comments, comment] }, + fetchedAt: 0, + error: undefined + }) + } + } + } }, - [workItem] + [workItem, detailsCacheKey] ) return ( @@ -2523,6 +2769,20 @@ export default function GitHubItemDialog({ localLabels={localLabels} onStateChange={setLocalState} onLabelsChange={setLocalLabels} + onMutated={() => { + // Why: drop the cached details for this item so the next + // open issues a fresh fetch instead of painting pre-edit + // state. We invalidate by (repoPath, type, number) match + // because a single mutation can affect entries across all + // issueSourcePreference values for the same number. + if (repoPath) { + invalidateWorkItemDetailsCacheByMatch({ + repoPath, + type: workItem.type, + number: workItem.number + }) + } + }} assignees={details?.assignees ?? []} onUse={onUse} />