fix: address review findings (#2379)
This commit is contained in:
Jinjing
GitHub
parent
c2c0969ae6
commit
ad14aa9d4e
|
|
@ -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<ReadonlySet<string>>(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<GitHubWorkItem[][]>(() => {
|
||||
|
|
@ -2193,6 +2201,7 @@ export default function TaskPage(): React.JSX.Element {
|
|||
ReadonlySet<string>
|
||||
>(() => new Set())
|
||||
const lastLinearRequestRef = useRef<{ nonce: number; signature: string } | null>(null)
|
||||
const landingLinearRefreshKeysRef = useRef<ReadonlySet<string>>(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) => {
|
||||
|
|
|
|||
|
|
@ -7,8 +7,13 @@ import {
|
|||
buildTaskPageRepoSourceState,
|
||||
findTaskPageDialogWorkItem,
|
||||
findTaskPageLinearDrawerIssue,
|
||||
reconcileTaskPageItemsAfterLandingRefresh,
|
||||
reconcileTaskPageLinearIssuesAfterLandingRefresh,
|
||||
reconcileTaskPagePagesAfterLandingRefresh,
|
||||
reconcileTaskPagePagesWithWorkItemsCache,
|
||||
selectTaskPageWorkItemsCacheEntries
|
||||
selectTaskPageWorkItemsCacheEntries,
|
||||
shouldResetTaskPagePaginationAfterLandingRefresh,
|
||||
shouldReplaceTaskPageItemsAfterRefresh
|
||||
} from './task-page-cache-selectors'
|
||||
|
||||
function entry<T>(data: T): CacheEntry<T> {
|
||||
|
|
@ -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')
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in New Issue