From ad14aa9d4eeaca1d2905eca0a13609198d6c685e Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Tue, 19 May 2026 17:55:59 -0700 Subject: [PATCH] fix: address review findings (#2379) --- src/renderer/src/components/TaskPage.tsx | 63 +++++- .../task-page-cache-selectors.test.ts | 111 ++++++++++- .../components/task-page-cache-selectors.ts | 181 ++++++++++++++++++ 3 files changed, 347 insertions(+), 8 deletions(-) diff --git a/src/renderer/src/components/TaskPage.tsx b/src/renderer/src/components/TaskPage.tsx index f0e615c0a..28bcba6ed 100644 --- a/src/renderer/src/components/TaskPage.tsx +++ b/src/renderer/src/components/TaskPage.tsx @@ -107,8 +107,12 @@ import { buildTaskPageRepoSourceState, findTaskPageDialogWorkItem, findTaskPageLinearIssue, + reconcileTaskPageLinearIssuesAfterLandingRefresh, + reconcileTaskPagePagesAfterLandingRefresh, reconcileTaskPagePagesWithWorkItemsCache, + shouldResetTaskPagePaginationAfterLandingRefresh, selectTaskPageWorkItemsCacheEntries, + shouldReplaceTaskPageItemsAfterRefresh, type TaskPageRepoSourceState } from '@/components/task-page-cache-selectors' import { deriveTaskPagePRCheckSummary } from '@/components/task-page-pr-check-summary' @@ -1913,6 +1917,10 @@ export default function TaskPage(): React.JSX.Element { // collapse onto a stale in-flight request that resolved against the // pre-flip source). const lastFetchedInvalidationNonceRef = useRef(0) + // Why: entering Tasks with fresh cache should still verify remote status + // once, but the result is reconciled into existing rows to avoid a full + // table shuffle when only status/key fields changed. + const landingGitHubRefreshKeysRef = useRef>(new Set()) // Why: pages holds all fetched pages of work items. Page 0 is seeded from // cache for instant first paint; subsequent pages are loaded via date cursors. const [pages, setPages] = useState(() => { @@ -2193,6 +2201,7 @@ export default function TaskPage(): React.JSX.Element { ReadonlySet >(() => new Set()) const lastLinearRequestRef = useRef<{ nonce: number; signature: string } | null>(null) + const landingLinearRefreshKeysRef = useRef>(new Set()) useEffect(() => { if (taskResumeAppliedRef.current || !persistedUIReady || !settings) { @@ -2883,11 +2892,13 @@ export default function TaskPage(): React.JSX.Element { // to this pre-paint; the fetch will fill it in. const preMerged: GitHubWorkItem[] = [] let anyUncached = false + let anyRepoCached = false for (const r of selectedRepos) { const cached = getCachedWorkItems(r.id, PER_REPO_FETCH_LIMIT, q) if (cached === null) { anyUncached = true } else { + anyRepoCached = true preMerged.push(...cached) } } @@ -2918,12 +2929,21 @@ export default function TaskPage(): React.JSX.Element { workItemsInvalidationNonce !== lastFetchedInvalidationNonceRef.current lastFetchedInvalidationNonceRef.current = workItemsInvalidationNonce const forcedFetch = (forceRefresh && taskRefreshNonce > 0) || preferenceInvalidated + const repoArgs = selectedRepos.map((r) => ({ repoId: r.id, path: r.path })) + const landingRefreshKey = `${repoArgs.map((r) => `${r.repoId}:${r.path}`).join('|')}::${q}` + const shouldProbeOnLanding = + !forcedFetch && anyRepoCached && !landingGitHubRefreshKeysRef.current.has(landingRefreshKey) + if (shouldProbeOnLanding) { + landingGitHubRefreshKeysRef.current = new Set([ + ...landingGitHubRefreshKeysRef.current, + landingRefreshKey + ]) + } // Why: manual refresh keeps cached rows visible, so the normal // `tasksLoading` flag may stay false. Track the forced fetch separately // so the toolbar still shows a refresh-in-progress affordance. setTasksRefreshing(forcedFetch) - const repoArgs = selectedRepos.map((r) => ({ repoId: r.id, path: r.path })) // Why: snapshot the retrying paths at effect-dispatch so overlapping // retries don't clear each other's pending state. An earlier cancelled // effect settling after a newer retry starts would otherwise wipe the @@ -2931,7 +2951,7 @@ export default function TaskPage(): React.JSX.Element { // when this effect dispatched preserves later additions. const dispatchedRetryPaths = retryingRepoPaths void fetchWorkItemsAcrossRepos(repoArgs, PER_REPO_FETCH_LIMIT, CROSS_REPO_DISPLAY_LIMIT, q, { - force: forcedFetch + force: forcedFetch || shouldProbeOnLanding }) .then(({ items, failedCount: failed }) => { // Why: clear only the repos this effect was responsible for @@ -2953,8 +2973,17 @@ export default function TaskPage(): React.JSX.Element { if (cancelled) { return } - setPages([items]) - setCurrentPage(0) + if (shouldProbeOnLanding) { + const replaceFirstPage = shouldReplaceTaskPageItemsAfterRefresh(page0, items) + const resetPagination = shouldResetTaskPagePaginationAfterLandingRefresh(page0, items) + setPages((current) => reconcileTaskPagePagesAfterLandingRefresh(current, items)) + if (replaceFirstPage || resetPagination) { + setCurrentPage(0) + } + } else { + setPages([items]) + setCurrentPage(0) + } setFailedCount(failed) setTasksLoading(false) setTasksRefreshing(false) @@ -3424,6 +3453,16 @@ export default function TaskPage(): React.JSX.Element { previousRequest?.nonce !== linearRefreshNonce && previousRequest?.signature === requestSignature lastLinearRequestRef.current = { nonce: linearRefreshNonce, signature: requestSignature } + const shouldProbeOnLanding = + !forceRefresh && + cachedIssues !== null && + !landingLinearRefreshKeysRef.current.has(requestSignature) + if (shouldProbeOnLanding) { + landingLinearRefreshKeysRef.current = new Set([ + ...landingLinearRefreshKeysRef.current, + requestSignature + ]) + } // Why: cached rows should remain visible on navigation. Only an explicit // refresh or a true cache miss needs the blocking loading state. @@ -3431,15 +3470,25 @@ export default function TaskPage(): React.JSX.Element { const request = readArgs.kind === 'search' - ? searchLinearIssues(readArgs.query, LINEAR_ITEM_LIMIT, { force: forceRefresh }) - : listLinearIssues(readArgs.filter, LINEAR_ITEM_LIMIT, { force: forceRefresh }) + ? searchLinearIssues(readArgs.query, LINEAR_ITEM_LIMIT, { + force: forceRefresh || shouldProbeOnLanding + }) + : listLinearIssues(readArgs.filter, LINEAR_ITEM_LIMIT, { + force: forceRefresh || shouldProbeOnLanding + }) void request .then((issues) => { if (cancelled) { return } - setLinearIssues(issues) + if (shouldProbeOnLanding) { + setLinearIssues((current) => + reconcileTaskPageLinearIssuesAfterLandingRefresh(current, issues) + ) + } else { + setLinearIssues(issues) + } setLinearLoading(false) }) .catch((err) => { diff --git a/src/renderer/src/components/task-page-cache-selectors.test.ts b/src/renderer/src/components/task-page-cache-selectors.test.ts index 5c73332c7..fb4e64183 100644 --- a/src/renderer/src/components/task-page-cache-selectors.test.ts +++ b/src/renderer/src/components/task-page-cache-selectors.test.ts @@ -7,8 +7,13 @@ import { buildTaskPageRepoSourceState, findTaskPageDialogWorkItem, findTaskPageLinearDrawerIssue, + reconcileTaskPageItemsAfterLandingRefresh, + reconcileTaskPageLinearIssuesAfterLandingRefresh, + reconcileTaskPagePagesAfterLandingRefresh, reconcileTaskPagePagesWithWorkItemsCache, - selectTaskPageWorkItemsCacheEntries + selectTaskPageWorkItemsCacheEntries, + shouldResetTaskPagePaginationAfterLandingRefresh, + shouldReplaceTaskPageItemsAfterRefresh } from './task-page-cache-selectors' function entry(data: T): CacheEntry { @@ -94,6 +99,110 @@ describe('task page cache selectors', () => { expect(nextPages[0][1]).toBe(otherRepoSameId) }) + it('merges landing refresh status changes without reordering GitHub rows', () => { + const first = { + ...workItem('issue-1', 'repo-1'), + state: 'open' as const, + updatedAt: '2026-01-01' + } + const second = { + ...workItem('issue-2', 'repo-1'), + state: 'open' as const, + updatedAt: '2026-01-02' + } + const refreshedSecond = { ...second, updatedAt: '2026-01-04' } + const refreshedFirst = { ...first, state: 'closed' as const, updatedAt: '2026-01-03' } + + const next = reconcileTaskPageItemsAfterLandingRefresh( + [first, second], + [refreshedSecond, refreshedFirst] + ) + + expect( + shouldReplaceTaskPageItemsAfterRefresh([first, second], [refreshedSecond, refreshedFirst]) + ).toBe(false) + expect(next).toEqual([refreshedFirst, refreshedSecond]) + }) + + it('replaces GitHub landing refresh rows when membership changes', () => { + const first = workItem('issue-1', 'repo-1') + const second = workItem('issue-2', 'repo-1') + const third = workItem('issue-3', 'repo-1') + const older = workItem('issue-4', 'repo-1') + + const nextPages = reconcileTaskPagePagesAfterLandingRefresh( + [[first, second], [older]], + [third, first] + ) + + expect(nextPages).toEqual([[third, first]]) + }) + + it('resets GitHub landing refresh pagination when first-page order changes', () => { + const first = { ...workItem('issue-1', 'repo-1'), updatedAt: '2026-01-02' } + const second = { ...workItem('issue-2', 'repo-1'), updatedAt: '2026-01-01' } + const older = { ...workItem('issue-3', 'repo-1'), updatedAt: '2025-12-31' } + const refreshedSecond = { ...second, updatedAt: '2026-01-03' } + + const nextPages = reconcileTaskPagePagesAfterLandingRefresh( + [[first, second], [older]], + [refreshedSecond, first] + ) + + expect( + shouldResetTaskPagePaginationAfterLandingRefresh([first, second], [refreshedSecond, first]) + ).toBe(true) + expect(nextPages).toEqual([[refreshedSecond, first]]) + }) + + it('resets GitHub landing refresh pagination when the cursor boundary changes', () => { + const first = { ...workItem('issue-1', 'repo-1'), updatedAt: '2026-01-03' } + const second = { ...workItem('issue-2', 'repo-1'), updatedAt: '2026-01-01' } + const older = { ...workItem('issue-3', 'repo-1'), updatedAt: '2025-12-31' } + const refreshedSecond = { ...second, updatedAt: '2026-01-02' } + + const nextPages = reconcileTaskPagePagesAfterLandingRefresh( + [[first, second], [older]], + [first, refreshedSecond] + ) + + expect(nextPages).toEqual([[first, refreshedSecond]]) + }) + + it('merges Linear landing refresh status changes without reordering issues', () => { + const first = { + ...linearIssue('LIN-1'), + identifier: 'ENG-1', + url: 'https://linear.test/ENG-1', + state: { name: 'Todo', type: 'unstarted', color: '#111111' }, + team: { id: 'team-1', name: 'Team', key: 'ENG' }, + labels: [], + labelIds: [], + priority: 2, + updatedAt: '2026-01-01' + } as LinearIssue + const second = { + ...first, + id: 'LIN-2', + identifier: 'ENG-2', + title: 'LIN-2', + updatedAt: '2026-01-02' + } + const refreshedFirst = { + ...first, + state: { name: 'Done', type: 'completed', color: '#222222' }, + updatedAt: '2026-01-03' + } + const refreshedSecond = { ...second, updatedAt: '2026-01-04' } + + const next = reconcileTaskPageLinearIssuesAfterLandingRefresh( + [first, second], + [refreshedSecond, refreshedFirst] + ) + + expect(next).toEqual([refreshedFirst, refreshedSecond]) + }) + it('returns null while the Linear drawer is closed and finds open issues by stable reference', () => { const issue = linearIssue('LIN-1') const searchIssue = linearIssue('LIN-2') diff --git a/src/renderer/src/components/task-page-cache-selectors.ts b/src/renderer/src/components/task-page-cache-selectors.ts index 99bda1ffb..ae8f65dc4 100644 --- a/src/renderer/src/components/task-page-cache-selectors.ts +++ b/src/renderer/src/components/task-page-cache-selectors.ts @@ -84,6 +84,187 @@ export function reconcileTaskPagePagesWithWorkItemsCache( return changed ? nextPages : (pages as GitHubWorkItem[][]) } +function taskPageWorkItemKey(item: GitHubWorkItem): string { + return `${item.repoId}\u0000${item.id}` +} + +function sortedStrings(values: readonly string[] | undefined): string { + return [...(values ?? [])].sort().join('\u0000') +} + +function sortedLogins(users: readonly { login: string | null | undefined }[] | undefined): string { + return [...(users ?? [])] + .map((user) => user.login ?? '') + .sort() + .join('\u0000') +} + +function taskPageWorkItemStatusSignature(item: GitHubWorkItem): string { + return JSON.stringify([ + item.type, + item.number, + item.title, + item.state, + item.url, + item.author, + item.branchName ?? null, + item.baseRefName ?? null, + sortedStrings(item.labels), + sortedLogins(item.assignees), + sortedLogins(item.reviewRequests), + item.reviewDecision ?? null, + item.checksSummary?.state ?? null, + item.checksSummary?.total ?? null, + item.checksSummary?.failed ?? null, + item.checksSummary?.pending ?? null, + item.mergeable ?? null, + item.mergeStateStatus ?? null, + item.updatedAt + ]) +} + +function taskPageWorkItemKeyOrderSignature(items: readonly GitHubWorkItem[]): string { + return items.map(taskPageWorkItemKey).join('\u0000') +} + +function taskPageWorkItemPaginationBoundary(items: readonly GitHubWorkItem[]): string | null { + return items.at(-1)?.updatedAt ?? null +} + +export function shouldReplaceTaskPageItemsAfterRefresh( + currentItems: readonly GitHubWorkItem[], + refreshedItems: readonly GitHubWorkItem[] +): boolean { + if (currentItems.length !== refreshedItems.length) { + return true + } + const currentKeys = new Set(currentItems.map(taskPageWorkItemKey)) + for (const item of refreshedItems) { + if (!currentKeys.has(taskPageWorkItemKey(item))) { + return true + } + } + return false +} + +export function reconcileTaskPageItemsAfterLandingRefresh( + currentItems: readonly GitHubWorkItem[], + refreshedItems: readonly GitHubWorkItem[] +): GitHubWorkItem[] { + if (shouldReplaceTaskPageItemsAfterRefresh(currentItems, refreshedItems)) { + return [...refreshedItems] + } + + const refreshedByKey = new Map(refreshedItems.map((item) => [taskPageWorkItemKey(item), item])) + let changed = false + const next = currentItems.map((item) => { + const refreshed = refreshedByKey.get(taskPageWorkItemKey(item)) + if ( + !refreshed || + taskPageWorkItemStatusSignature(item) === taskPageWorkItemStatusSignature(refreshed) + ) { + return item + } + changed = true + return refreshed + }) + return changed ? next : (currentItems as GitHubWorkItem[]) +} + +export function shouldResetTaskPagePaginationAfterLandingRefresh( + currentFirstPage: readonly GitHubWorkItem[], + refreshedItems: readonly GitHubWorkItem[] +): boolean { + if (shouldReplaceTaskPageItemsAfterRefresh(currentFirstPage, refreshedItems)) { + return true + } + if ( + taskPageWorkItemKeyOrderSignature(currentFirstPage) !== + taskPageWorkItemKeyOrderSignature(refreshedItems) + ) { + return true + } + return ( + taskPageWorkItemPaginationBoundary(currentFirstPage) !== + taskPageWorkItemPaginationBoundary(refreshedItems) + ) +} + +export function reconcileTaskPagePagesAfterLandingRefresh( + pages: readonly GitHubWorkItem[][], + refreshedItems: readonly GitHubWorkItem[] +): GitHubWorkItem[][] { + const firstPage = pages[0] ?? [] + if (shouldResetTaskPagePaginationAfterLandingRefresh(firstPage, refreshedItems)) { + return [[...refreshedItems]] + } + const nextFirstPage = reconcileTaskPageItemsAfterLandingRefresh(firstPage, refreshedItems) + if (nextFirstPage === firstPage) { + return pages as GitHubWorkItem[][] + } + return [nextFirstPage, ...pages.slice(1)] +} + +function linearIssueKey(issue: LinearIssue): string { + return issue.id +} + +function linearIssueStatusSignature(issue: LinearIssue): string { + return JSON.stringify([ + issue.identifier, + issue.title, + issue.url, + issue.state.name, + issue.state.type, + issue.state.color, + issue.team.id, + issue.team.name, + issue.team.key, + sortedStrings(issue.labels), + issue.assignee?.id ?? null, + issue.assignee?.displayName ?? null, + issue.priority, + issue.updatedAt + ]) +} + +export function shouldReplaceTaskPageLinearIssuesAfterRefresh( + currentIssues: readonly LinearIssue[], + refreshedIssues: readonly LinearIssue[] +): boolean { + if (currentIssues.length !== refreshedIssues.length) { + return true + } + const currentKeys = new Set(currentIssues.map(linearIssueKey)) + for (const issue of refreshedIssues) { + if (!currentKeys.has(linearIssueKey(issue))) { + return true + } + } + return false +} + +export function reconcileTaskPageLinearIssuesAfterLandingRefresh( + currentIssues: readonly LinearIssue[], + refreshedIssues: readonly LinearIssue[] +): LinearIssue[] { + if (shouldReplaceTaskPageLinearIssuesAfterRefresh(currentIssues, refreshedIssues)) { + return [...refreshedIssues] + } + + const refreshedByKey = new Map(refreshedIssues.map((issue) => [linearIssueKey(issue), issue])) + let changed = false + const next = currentIssues.map((issue) => { + const refreshed = refreshedByKey.get(linearIssueKey(issue)) + if (!refreshed || linearIssueStatusSignature(issue) === linearIssueStatusSignature(refreshed)) { + return issue + } + changed = true + return refreshed + }) + return changed ? next : (currentIssues as LinearIssue[]) +} + export function findTaskPageDialogWorkItem( workItemsCache: WorkItemsCache, dialogWorkItemKey: TaskPageDialogWorkItemKey