mirror of
https://github.com/stablyai/orca.git
synced 2026-10-30 16:47:34 +00:00
fix(github): ignore overlapping resolve on an in-flight review thread (#35655)
Track in-flight resolve/unresolve requests per comments cacheKey + threadId in the store. A second request on the same thread no-ops until the first settles, and the Checks panel ResolveButton shows pending from the store so the remounted button after the optimistic section move cannot fire the opposite request. Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com>
This commit is contained in:
@@ -18,6 +18,8 @@ import {
|
||||
DropdownMenuTrigger
|
||||
} from '@/components/ui/dropdown-menu'
|
||||
import { cn } from '@/lib/utils'
|
||||
import { useAppStore } from '@/store'
|
||||
import { isReviewThreadResolutionInFlight } from '@/store/github/review-thread-resolution-in-flight'
|
||||
import { basename } from '@/lib/path'
|
||||
import { isBotPRComment } from '../../../../../shared/pr-comment-audience'
|
||||
import { setPRBotAuthorOverride } from '@/lib/pr-bot-author-overrides'
|
||||
@@ -99,6 +101,8 @@ export function ResolveButton({
|
||||
onResolve: (threadId: string, resolve: boolean) => boolean | Promise<boolean>
|
||||
}): React.JSX.Element {
|
||||
const [loading, setLoading] = useState(false)
|
||||
// Why: the optimistic move remounts this button in the other section, losing local loading state.
|
||||
const inFlight = useAppStore((s) => isReviewThreadResolutionInFlight(s, threadId))
|
||||
const loadingResetTimerRef = useRef<number | null>(null)
|
||||
|
||||
const clearLoadingResetTimer = useCallback((): void => {
|
||||
@@ -129,7 +133,7 @@ export function ResolveButton({
|
||||
|
||||
return (
|
||||
<span ref={setResolveButtonRootRef} className="contents">
|
||||
{loading ? (
|
||||
{loading || inFlight ? (
|
||||
<LoaderCircle className="size-3 animate-spin text-muted-foreground shrink-0" />
|
||||
) : (
|
||||
<button
|
||||
|
||||
+5
@@ -21,6 +21,7 @@ import {
|
||||
import type { PRComment } from '../../../../../shared/github/comment-types'
|
||||
import { translate } from '@/i18n/i18n'
|
||||
import { useAppStore } from '@/store'
|
||||
import { isReviewThreadResolutionInFlight } from '@/store/github/review-thread-resolution-in-flight'
|
||||
import { worktreeLaunchWorkspace } from '@/lib/agent-launch-route-input'
|
||||
|
||||
type ChecksPanelCommentResolutionInput = Pick<
|
||||
@@ -106,6 +107,10 @@ export function useChecksPanelCommentResolution(model: ChecksPanelCommentResolut
|
||||
if (!repo || !prNumber) {
|
||||
return false
|
||||
}
|
||||
// Why: skip the local optimistic flip too; the store no-ops overlapping requests.
|
||||
if (isReviewThreadResolutionInFlight(useAppStore.getState(), threadId)) {
|
||||
return true
|
||||
}
|
||||
const requestKey = checksPanelAsyncResultKey(
|
||||
prCacheKey,
|
||||
branch,
|
||||
|
||||
@@ -12,6 +12,10 @@ import {
|
||||
getGitHubSourceTarget,
|
||||
getGitHubWorkItemRequestContext
|
||||
} from './work-item-routing'
|
||||
import {
|
||||
reviewThreadInFlightKey,
|
||||
withReviewThreadInFlight
|
||||
} from './review-thread-resolution-in-flight'
|
||||
|
||||
export const createReviewThreadActions = (
|
||||
set: Parameters<StateCreator<AppState>>[0],
|
||||
@@ -34,77 +38,88 @@ export const createReviewThreadActions = (
|
||||
repo !== undefined
|
||||
)
|
||||
|
||||
const previousThreadComments =
|
||||
get().commentsCache[cacheKey]?.data?.filter((comment) => comment.threadId === threadId) ?? []
|
||||
if (previousThreadComments.length > 0) {
|
||||
set((s) => {
|
||||
const entry = s.commentsCache[cacheKey]
|
||||
if (!entry?.data) {
|
||||
return s
|
||||
}
|
||||
return {
|
||||
commentsCache: {
|
||||
...s.commentsCache,
|
||||
[cacheKey]: {
|
||||
...entry,
|
||||
data: markPRCommentThreadResolved(entry.data, threadId, resolve)
|
||||
const inFlightKey = reviewThreadInFlightKey(cacheKey, threadId)
|
||||
// Why: overlapping opposite requests can settle in either order; true keeps callers from toasting or rolling back.
|
||||
if (get().reviewThreadResolutionsInFlight[inFlightKey] !== undefined) {
|
||||
return true
|
||||
}
|
||||
set((s) => withReviewThreadInFlight(s, inFlightKey, threadId))
|
||||
try {
|
||||
const previousThreadComments =
|
||||
get().commentsCache[cacheKey]?.data?.filter((comment) => comment.threadId === threadId) ??
|
||||
[]
|
||||
if (previousThreadComments.length > 0) {
|
||||
set((s) => {
|
||||
const entry = s.commentsCache[cacheKey]
|
||||
if (!entry?.data) {
|
||||
return s
|
||||
}
|
||||
return {
|
||||
commentsCache: {
|
||||
...s.commentsCache,
|
||||
[cacheKey]: {
|
||||
...entry,
|
||||
data: markPRCommentThreadResolved(entry.data, threadId, resolve)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
const requestContext = getGitHubWorkItemRequestContext(
|
||||
get(),
|
||||
getGitHubSourceTarget(get(), repo, options?.sourceContext),
|
||||
repoId ?? repoPath,
|
||||
repoPath,
|
||||
options?.sourceContext
|
||||
)
|
||||
let ok = false
|
||||
try {
|
||||
ok =
|
||||
requestContext.target.kind === 'environment'
|
||||
? await callRuntimeRpc<boolean>(
|
||||
{ kind: 'environment', environmentId: requestContext.target.environmentId },
|
||||
'github.resolveReviewThread',
|
||||
{
|
||||
repo: requestContext.target.runtimeRepoId,
|
||||
const requestContext = getGitHubWorkItemRequestContext(
|
||||
get(),
|
||||
getGitHubSourceTarget(get(), repo, options?.sourceContext),
|
||||
repoId ?? repoPath,
|
||||
repoPath,
|
||||
options?.sourceContext
|
||||
)
|
||||
let ok = false
|
||||
try {
|
||||
ok =
|
||||
requestContext.target.kind === 'environment'
|
||||
? await callRuntimeRpc<boolean>(
|
||||
{ kind: 'environment', environmentId: requestContext.target.environmentId },
|
||||
'github.resolveReviewThread',
|
||||
{
|
||||
repo: requestContext.target.runtimeRepoId,
|
||||
threadId,
|
||||
resolve,
|
||||
prRepo: options?.prRepo ?? null
|
||||
},
|
||||
{ timeoutMs: 30_000 }
|
||||
)
|
||||
: await window.api.gh.resolveReviewThread({
|
||||
repoPath,
|
||||
repoId,
|
||||
threadId,
|
||||
resolve,
|
||||
prRepo: options?.prRepo ?? null
|
||||
},
|
||||
{ timeoutMs: 30_000 }
|
||||
)
|
||||
: await window.api.gh.resolveReviewThread({
|
||||
repoPath,
|
||||
repoId,
|
||||
threadId,
|
||||
resolve,
|
||||
prRepo: options?.prRepo ?? null,
|
||||
sourceContext: options?.sourceContext
|
||||
})
|
||||
} catch (err) {
|
||||
console.error('Failed to update review thread:', err)
|
||||
ok = false
|
||||
}
|
||||
if (!ok && previousThreadComments.length > 0) {
|
||||
set((s) => {
|
||||
const entry = s.commentsCache[cacheKey]
|
||||
if (!entry?.data) {
|
||||
return s
|
||||
}
|
||||
return {
|
||||
commentsCache: {
|
||||
...s.commentsCache,
|
||||
[cacheKey]: {
|
||||
...entry,
|
||||
data: restorePRCommentThreadSnapshot(entry.data, previousThreadComments)
|
||||
prRepo: options?.prRepo ?? null,
|
||||
sourceContext: options?.sourceContext
|
||||
})
|
||||
} catch (err) {
|
||||
console.error('Failed to update review thread:', err)
|
||||
ok = false
|
||||
}
|
||||
if (!ok && previousThreadComments.length > 0) {
|
||||
set((s) => {
|
||||
const entry = s.commentsCache[cacheKey]
|
||||
if (!entry?.data) {
|
||||
return s
|
||||
}
|
||||
return {
|
||||
commentsCache: {
|
||||
...s.commentsCache,
|
||||
[cacheKey]: {
|
||||
...entry,
|
||||
data: restorePRCommentThreadSnapshot(entry.data, previousThreadComments)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
})
|
||||
})
|
||||
}
|
||||
return ok
|
||||
} finally {
|
||||
set((s) => withReviewThreadInFlight(s, inFlightKey, null))
|
||||
}
|
||||
return ok
|
||||
}
|
||||
})
|
||||
|
||||
@@ -0,0 +1,26 @@
|
||||
import type { GitHubSlice } from './slice-types'
|
||||
|
||||
type InFlightState = Pick<GitHubSlice, 'reviewThreadResolutionsInFlight'>
|
||||
|
||||
export function reviewThreadInFlightKey(commentsCacheKey: string, threadId: string): string {
|
||||
return `${commentsCacheKey}\u0000${threadId}`
|
||||
}
|
||||
|
||||
/** Thread node IDs are globally unique, so UI callers without a cache key can match on threadId. */
|
||||
export function isReviewThreadResolutionInFlight(state: InFlightState, threadId: string): boolean {
|
||||
return Object.values(state.reviewThreadResolutionsInFlight).includes(threadId)
|
||||
}
|
||||
|
||||
export function withReviewThreadInFlight(
|
||||
state: InFlightState,
|
||||
key: string,
|
||||
threadId: string | null
|
||||
): InFlightState {
|
||||
const next = { ...state.reviewThreadResolutionsInFlight }
|
||||
if (threadId === null) {
|
||||
delete next[key]
|
||||
} else {
|
||||
next[key] = threadId
|
||||
}
|
||||
return { reviewThreadResolutionsInFlight: next }
|
||||
}
|
||||
@@ -47,6 +47,8 @@ export type GitHubSlice = {
|
||||
issueCache: Record<string, CacheEntry<IssueInfo>>
|
||||
checksCache: Record<string, CacheEntry<PRCheckDetail[]>>
|
||||
commentsCache: Record<string, CacheEntry<PRComment[]>>
|
||||
/** Resolve/unresolve requests in flight: comments cacheKey + threadId -> threadId. */
|
||||
reviewThreadResolutionsInFlight: Record<string, string>
|
||||
prRefreshSequences: Record<string, number>
|
||||
prRefreshStates: Record<string, PRRefreshState>
|
||||
prVisibleRefreshGeneration: number
|
||||
|
||||
@@ -55,3 +55,34 @@ it('keeps a reply posted while a failed unresolve was pending, with the thread s
|
||||
{ ...restReply, isResolved: true }
|
||||
])
|
||||
})
|
||||
|
||||
it('ignores an opposite resolve on a thread while the first request is in flight', async () => {
|
||||
const store = makeStore()
|
||||
const first = Promise.withResolvers<boolean>()
|
||||
mockApi.gh.resolveReviewThread.mockReturnValueOnce(first.promise)
|
||||
const pending = store.getState().resolveReviewThread(repoPath, 12, threadId, true, options)
|
||||
await expect(
|
||||
store.getState().resolveReviewThread(repoPath, 12, threadId, false, options)
|
||||
).resolves.toBe(true)
|
||||
expect(mockApi.gh.resolveReviewThread).toHaveBeenCalledTimes(1)
|
||||
expect(store.getState().commentsCache[cacheKey]?.data?.[0].isResolved).toBe(true)
|
||||
first.resolve(true)
|
||||
await expect(pending).resolves.toBe(true)
|
||||
})
|
||||
|
||||
it('releases the thread after a failed request so it can be resolved again', async () => {
|
||||
const store = makeStore()
|
||||
const first = Promise.withResolvers<boolean>()
|
||||
mockApi.gh.resolveReviewThread.mockReturnValueOnce(first.promise)
|
||||
const pending = store.getState().resolveReviewThread(repoPath, 12, threadId, true, options)
|
||||
await store.getState().resolveReviewThread(repoPath, 12, threadId, true, options)
|
||||
first.reject(new Error('socket closed'))
|
||||
await expect(pending).resolves.toBe(false)
|
||||
expect(store.getState().reviewThreadResolutionsInFlight).toEqual({})
|
||||
mockApi.gh.resolveReviewThread.mockResolvedValueOnce(true)
|
||||
await expect(
|
||||
store.getState().resolveReviewThread(repoPath, 12, threadId, true, options)
|
||||
).resolves.toBe(true)
|
||||
expect(mockApi.gh.resolveReviewThread).toHaveBeenCalledTimes(2)
|
||||
expect(store.getState().commentsCache[cacheKey]?.data?.[0].isResolved).toBe(true)
|
||||
})
|
||||
|
||||
@@ -35,6 +35,7 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (
|
||||
issueCache: {},
|
||||
checksCache: {},
|
||||
commentsCache: {},
|
||||
reviewThreadResolutionsInFlight: {},
|
||||
prRefreshSequences: {},
|
||||
prRefreshStates: {},
|
||||
prVisibleRefreshGeneration: 0,
|
||||
|
||||
Reference in New Issue
Block a user