feat(web-ui): PROOF9-gated merge button on PR Status panel#583
feat(web-ui): PROOF9-gated merge button on PR Status panel#583
Conversation
Add a merge button to PRStatusPanel that enforces two gates before
enabling: all PROOF9 requirements must be satisfied/waived and all CI
checks must pass. Polls proof status via proofApi.getStatus alongside
the existing PR status polling. Shows inline blocking messages with
links to open requirements when PROOF9 is blocking, and CI status text
when checks are pending or failing. On successful merge, displays a
confirmation banner and stops all polling.
- Add MergeResponse / MergePRRequest types
- Add prApi.merge() calling POST /api/v2/pr/{n}/merge
- Extend PRStatusPanel with proof SWR hook, gate logic, merge button
- 9 Jest tests covering all acceptance criteria
|
Caution Review failedPull request was closed or merged during review WalkthroughPRStatusPanel now enforces a PROOF9 + CI merge gate: it fetches PROOF9 status, computes merge eligibility, calls a new PR merge API, and surfaces merge/in-flight/success/failure states and inline blocking reasons in the UI. Changes
Sequence DiagramsequenceDiagram
participant User
participant PRStatusPanel as PR Status Panel
participant prApi
participant proofApi
participant Server
User->>PRStatusPanel: Open PR page
PRStatusPanel->>prApi: getStatus(prNumber)
PRStatusPanel->>proofApi: getStatus(workspacePath)
proofApi-->>PRStatusPanel: proof requirements
prApi-->>PRStatusPanel: CI check data
PRStatusPanel->>PRStatusPanel: Compute canMerge (CI passed && no open PROOF9)
alt Gates Cleared
User->>PRStatusPanel: Click Merge
PRStatusPanel->>PRStatusPanel: Set isMerging = true (button disabled, label "merging")
PRStatusPanel->>prApi: merge(workspacePath, prNumber, { method })
prApi->>Server: POST /api/v2/pr/merge
alt Merge Success
Server-->>prApi: MergeResponse (merged: true)
prApi-->>PRStatusPanel: success
PRStatusPanel->>PRStatusPanel: Set merged = true, show success banner, stop polling
else Merge Failure
Server-->>prApi: Error (detail)
prApi-->>PRStatusPanel: error
PRStatusPanel->>PRStatusPanel: Set mergeError, show error banner, re-enable button
end
else Gates Blocked
PRStatusPanel->>User: Disable Merge button, show tooltip and inline blocking messages (PROOF9 list, CI status)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Review — feat(web-ui): PROOF9-gated merge button on PR Status panelOverall this is a clean, well-scoped implementation that follows the existing patterns in the codebase. The gate logic is correct and the test coverage hits all nine acceptance criteria. A couple of issues worth addressing before merge: Bug: error message never surfaces from axios rejections
} catch (err: unknown) {
const apiErr = err as { detail?: string };
setMergeError(apiErr?.detail ?? 'Merge failed. Please try again.');
}When The test sidesteps this by rejecting with a plain object Fix: } catch (err: unknown) {
const axiosDetail = (err as { response?: { data?: { detail?: string } } })?.response?.data?.detail;
const plainDetail = (err as { detail?: string })?.detail;
setMergeError(axiosDetail ?? plainDetail ?? 'Merge failed. Please try again.');
}And update the test to use an axios-shaped rejection so it stays aligned with real behavior. Bug: merge button enabled while PR/proof data is still loading
The simplest fix is to gate const canMerge =
!!data && !!proofData &&
openRequirements.length === 0 && ciPassing;Minor: TooltipContent can render emptyWhen <TooltipContent>
{openRequirements.length > 0 && 'Resolve all open PROOF9 requirements. '}
{ciFailing && 'Fix failing CI checks. '}
{ciPending && 'Wait for CI checks to complete.'}
{/* fallback if loading */}
{!openRequirements.length && !ciFailing && !ciPending && 'Loading status…'}
</TooltipContent>Nit:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
web-ui/src/components/review/PRStatusPanel.tsx (1)
128-140: Consider adding optimistic UI update or mutating SWR cache after successful merge.After a successful merge, the component sets
merged = truelocally but doesn't update the SWR cache. While the polling will eventually catch up, callingmutateon the PR status SWR key would immediately reflect the merged state if the user navigates away and back before the next poll.♻️ Example: Mutate SWR cache on merge success
- const { data, error } = useSWR<PRStatusResponse>( + const { data, error, mutate } = useSWR<PRStatusResponse>( swrKey, () => prApi.getStatus(workspacePath, prNumber), ... ); const handleMerge = async () => { setIsMerging(true); setMergeError(null); try { await prApi.merge(workspacePath, prNumber, { method: 'squash' }); setMerged(true); + // Optimistically update cache to reflect merged state + mutate((prev) => prev ? { ...prev, merge_state: 'merged' } : prev, false); } catch (err: unknown) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/components/review/PRStatusPanel.tsx` around lines 128 - 140, The handleMerge function updates local state (setMerged) after prApi.merge but doesn't update the SWR cache; after a successful merge (inside the try after await prApi.merge(...)), call the SWR mutate for the PR status key used elsewhere (the same key passed to useSWR) to update the cached PR state to merged (or revalidate) so the UI and other components reflect the merge immediately; ensure you import/use the correct mutate function or SWR hook and keep existing setIsMerging/setMergeError behavior.web-ui/__tests__/components/review/PRStatusPanel.test.tsx (2)
154-170: Consider awaiting the final state after resolving the merge promise.After calling
resolveMerge(...)at line 169, the test ends without awaiting the subsequent state update. This could lead to React state updates occurring after the test completes, potentially causing console warnings about unmounted component updates in CI.♻️ Proposed fix to await final state
resolveMerge({ sha: 'abc', merged: true, message: 'ok' }); + + // Wait for final state to settle + await waitFor(() => { + expect(screen.getByText(/merged successfully/i)).toBeInTheDocument(); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/__tests__/components/review/PRStatusPanel.test.tsx` around lines 154 - 170, The test resolves the in-flight merge promise via resolveMerge(...) but doesn't wait for the component to update, risking state updates after test completion; after calling resolveMerge({ sha: 'abc', merged: true, message: 'ok' }) you should await the UI settling (use waitFor) to assert the post-merge state (e.g., that the merge button text returns to "merge" or the button is enabled) so the test waits for PRStatusPanel to finish updating after mockMerge/mergePromise resolves.
82-90: Type safety improvement: consider typing the SWR mock return structure.The mock returns
as anywhich bypasses type checking. While functional for tests, this could mask issues if the SWR hook's return signature changes.♻️ Proposed typed mock return
const setupSWRMock = (prStatus: object, proofStatus: object) => { mockUseSWR.mockImplementation((key: unknown) => { const keyStr = typeof key === 'string' ? key : ''; if (keyStr.includes('/api/v2/proof/status')) { - return { data: proofStatus, error: undefined, isLoading: false, mutate: jest.fn() } as any; + return { data: proofStatus, error: undefined, isLoading: false, mutate: jest.fn() } as ReturnType<typeof useSWR>; } - return { data: prStatus, error: undefined, isLoading: false, mutate: jest.fn() } as any; + return { data: prStatus, error: undefined, isLoading: false, mutate: jest.fn() } as ReturnType<typeof useSWR>; }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/__tests__/components/review/PRStatusPanel.test.tsx` around lines 82 - 90, The setupSWRMock currently returns objects cast with "as any", losing type safety; update setupSWRMock and mockUseSWR to return a properly typed SWR response (e.g., use SWRResponse<T, E> from 'swr' or ReturnType<typeof useSWR>) instead of any, parameterize prStatus and proofStatus with appropriate generics, and cast each returned object to that typed SWRResponse (for the proof status branch and the default branch) so the mocked shape matches the real useSWR return type; update imports to include SWRResponse or use ReturnType<typeof useSWR> to ensure compilation if the hook signature changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web-ui/__tests__/components/review/PRStatusPanel.test.tsx`:
- Around line 154-170: The test resolves the in-flight merge promise via
resolveMerge(...) but doesn't wait for the component to update, risking state
updates after test completion; after calling resolveMerge({ sha: 'abc', merged:
true, message: 'ok' }) you should await the UI settling (use waitFor) to assert
the post-merge state (e.g., that the merge button text returns to "merge" or the
button is enabled) so the test waits for PRStatusPanel to finish updating after
mockMerge/mergePromise resolves.
- Around line 82-90: The setupSWRMock currently returns objects cast with "as
any", losing type safety; update setupSWRMock and mockUseSWR to return a
properly typed SWR response (e.g., use SWRResponse<T, E> from 'swr' or
ReturnType<typeof useSWR>) instead of any, parameterize prStatus and proofStatus
with appropriate generics, and cast each returned object to that typed
SWRResponse (for the proof status branch and the default branch) so the mocked
shape matches the real useSWR return type; update imports to include SWRResponse
or use ReturnType<typeof useSWR> to ensure compilation if the hook signature
changes.
In `@web-ui/src/components/review/PRStatusPanel.tsx`:
- Around line 128-140: The handleMerge function updates local state (setMerged)
after prApi.merge but doesn't update the SWR cache; after a successful merge
(inside the try after await prApi.merge(...)), call the SWR mutate for the PR
status key used elsewhere (the same key passed to useSWR) to update the cached
PR state to merged (or revalidate) so the UI and other components reflect the
merge immediately; ensure you import/use the correct mutate function or SWR hook
and keep existing setIsMerging/setMergeError behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4318ba46-ffc4-4454-9ebc-585afe8f92c1
📒 Files selected for processing (4)
web-ui/__tests__/components/review/PRStatusPanel.test.tsxweb-ui/src/components/review/PRStatusPanel.tsxweb-ui/src/lib/api.tsweb-ui/src/types/index.ts
- Mutate SWR cache on merge success for optimistic UI update - Await final state in in-flight test to prevent act() warning
Prevent merge button from appearing enabled during initial load by requiring both PR status and proof status data to be present before canMerge can be true.
Follow-up reviewThanks for the iteration. Checking against my previous two blocker items: ✅ Fixed — loading-state gate
Both data guards are in place. The merge button stays disabled while either SWR hook hasn't resolved yet. Good. Also noted: ❌ Still open — axios error message never surfacesThe catch block is unchanged: } catch (err: unknown) {
const apiErr = err as { detail?: string };
setMergeError(apiErr?.detail ?? 'Merge failed. Please try again.');
}When axios rejects, the thrown object is an The test still masks this because it rejects with a plain mockMerge.mockRejectedValueOnce({ detail: 'Cannot merge: conflicts detected' });Fix needed in the component: } catch (err: unknown) {
const axiosDetail = (err as { response?: { data?: { detail?: string } } })
?.response?.data?.detail;
const plainDetail = (err as { detail?: string })?.detail;
setMergeError(axiosDetail ?? plainDetail ?? 'Merge failed. Please try again.');
}And in the test, use an axios-shaped rejection so it stays honest: mockMerge.mockRejectedValueOnce({
response: { data: { detail: 'Cannot merge: conflicts detected' } },
});Minor — tooltip still renders empty during loading
{!canMerge && (
<TooltipContent>
{openRequirements.length > 0 && '...'}
{ciFailing && '...'}
{ciPending && '...'}
</TooltipContent>
)}While data is loading: Everything else looks good — the gate logic, SWR polling stop, inline PROOF9 links, and banner UX are solid. The axios error handling is the one remaining blocker before merge. |
Review: feat(web-ui): PROOF9-gated merge button on PR Status panelThis is a solid, well-scoped implementation of #571. The gate logic is clear, the test coverage is good, and the backend endpoint was already in place. A few items worth addressing before merge: Bug: PROOF9 section shows "All clear" while
|
1 participant
Summary
Implements #571: [Phase 4A] PR Status: PROOF9-gated merge button
PRStatusPanelwith a PROOF9-gated Merge buttonproofApi.getStatus(15 s) alongside existing PR status polling (30 s)/proof/{id}, CI status textChanges
web-ui/src/types/index.tsMergeResponse,MergePRRequesttypesweb-ui/src/lib/api.tsprApi.merge()→POST /api/v2/pr/{n}/mergeweb-ui/src/components/review/PRStatusPanel.tsxweb-ui/__tests__/components/review/PRStatusPanel.test.tsxAcceptance Criteria
npm testpass (756/756)Test Plan
npm run buildsucceedsImplementation Notes
PRStatusPanelfrom [Phase 4A] PR Status: live CI checks, review status, and merge state #570 rather than creating from scratchprApi.get— CI info already available fromprApi.getStatusCloses #571
Summary by CodeRabbit
New Features
Tests