Conversation
…w-up) - Show PROOF9 loading skeleton instead of false "All clear" while proofData is still loading - Surface proof API error in the PROOF9 section when fetch fails - Treat externally merged PRs (merge_state==='merged') the same as locally merged — show success banner, hide merge button - Add 4 new tests: ciPending path, proof loading, proof error, externally merged PR
WalkthroughEnhanced the PRStatusPanel component and tests to handle PROOF9 API loading and error states explicitly. Added logic to treat externally-merged PRs consistently via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Review: fix(pr-status) — post-merge follow-up (#571)These changes directly address the feedback from #583. Each fix is targeted, well-tested, and correct. A few observations: What's solid
One gap worth fixing before this mergesTooltip content is empty during proof-loading and proof-error states. When {!canMerge && (
<TooltipContent>
{openRequirements.length > 0 && 'Resolve all open PROOF9 requirements. '}
{ciFailing && 'Fix failing CI checks. '}
{ciPending && 'Wait for CI checks to complete.'}
{/* ← nothing renders when proof is loading/errored */}
</TooltipContent>
)}Suggested fix (minimal): {proofLoading && 'PROOF9 status loading — merge unavailable. '}
{proofError && !proofData && 'PROOF9 status unavailable — merge blocked until resolved. '}This pre-existed in Minor (non-blocking)The proof SWR { refreshInterval: merged ? 0 : 15_000 }An externally-merged PR will continue polling proof data at 15s. Low-impact since proof data is static post-merge, but using Good shape overall — the tooltip gap is the only behaviorally-visible issue. Fix that and this is ready. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-ui/src/components/review/PRStatusPanel.tsx (1)
100-104:⚠️ Potential issue | 🟠 MajorBlock merge on any PROOF9 fetch error, not only when
proofDatais absent.At Line 125, the merge gate checks
!!proofDatabut ignoresproofError. At Line 210, the error conditionproofError && !proofDatasuppresses the error message whenever staleproofDataexists. In SWR, after a successful fetch, a later revalidation failure can set botherroranddata(stale). This allows merging on stale PROOF9 data and hides the error state from the user.💡 Suggested fix
- const canMerge = !!data && !!proofData && openRequirements.length === 0 && ciPassing; + const proofBlocked = proofLoading || !!proofError || !proofData; + const canMerge = !!data && !proofBlocked && openRequirements.length === 0 && ciPassing;- ) : proofError && !proofData ? ( + ) : proofError ? ( <p className="text-xs text-muted-foreground"> Unable to load PROOF9 status — merge blocked until resolved. </p>🤖 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 100 - 104, The current logic allows a merge when stale PROOF9 data exists because it only checks !!proofData and hides errors when proofError is set alongside stale proofData; update the merge gate and error display to consider proofError: where the UI decides merge eligibility (the check using proofData and merged), require that proofError is falsy (e.g., only allow merge when !!proofData && !proofError && !proofLoading), and where the error banner/indicator is rendered (the condition using proofError && !proofData), change it to display whenever proofError is truthy (e.g., proofError) so fetch errors are surfaced even if stale proofData exists; reference the useSWR result variables proofData, proofError, proofLoading and the proofKey/proofApi.getStatus call to locate the logic to change.
🧹 Nitpick comments (1)
web-ui/src/components/review/PRStatusPanel.tsx (1)
103-104: Stop PROOF9 polling once the PR is externally merged/closed.At Line 103, polling stops only for local
merged. Consider using server merge state too, so externally merged PRs don’t keep polling.♻️ Suggested refactor
const { data: proofData, error: proofError, isLoading: proofLoading } = useSWR<ProofStatusResponse>( proofKey, () => proofApi.getStatus(workspacePath), - { refreshInterval: merged ? 0 : 15_000 } + { + refreshInterval: + merged || data?.merge_state === 'merged' || data?.merge_state === 'closed' ? 0 : 15_000, + } );Also applies to: 124-125
🤖 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 103 - 104, The polling currently stops only when the local `merged` flag is true; update the polling condition used for `refreshInterval` (and the similar check at the other spot) to also check the server-side PR state (e.g., `pr?.state` or whatever server field indicates merged/closed) so that polling is set to 0 when either local `merged` OR the server reports the PR as merged/closed; locate the `refreshInterval: merged ? 0 : 15_000` usage in PRStatusPanel (and the duplicate at the other spot) and change the condition to include the server-state check (for example `merged || pr?.state === 'MERGED' || pr?.state === 'CLOSED'`) so externally merged/closed PRs stop polling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@web-ui/src/components/review/PRStatusPanel.tsx`:
- Around line 100-104: The current logic allows a merge when stale PROOF9 data
exists because it only checks !!proofData and hides errors when proofError is
set alongside stale proofData; update the merge gate and error display to
consider proofError: where the UI decides merge eligibility (the check using
proofData and merged), require that proofError is falsy (e.g., only allow merge
when !!proofData && !proofError && !proofLoading), and where the error
banner/indicator is rendered (the condition using proofError && !proofData),
change it to display whenever proofError is truthy (e.g., proofError) so fetch
errors are surfaced even if stale proofData exists; reference the useSWR result
variables proofData, proofError, proofLoading and the
proofKey/proofApi.getStatus call to locate the logic to change.
---
Nitpick comments:
In `@web-ui/src/components/review/PRStatusPanel.tsx`:
- Around line 103-104: The polling currently stops only when the local `merged`
flag is true; update the polling condition used for `refreshInterval` (and the
similar check at the other spot) to also check the server-side PR state (e.g.,
`pr?.state` or whatever server field indicates merged/closed) so that polling is
set to 0 when either local `merged` OR the server reports the PR as
merged/closed; locate the `refreshInterval: merged ? 0 : 15_000` usage in
PRStatusPanel (and the duplicate at the other spot) and change the condition to
include the server-state check (for example `merged || pr?.state === 'MERGED' ||
pr?.state === 'CLOSED'`) so externally merged/closed PRs stop polling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74f090ed-c249-4aff-97b7-8790f51e1fcb
📒 Files selected for processing (2)
web-ui/__tests__/components/review/PRStatusPanel.test.tsxweb-ui/src/components/review/PRStatusPanel.tsx
1 participant
Summary
Addresses claude-review feedback from PR #583 that was posted after merge.
proofDatais loadingmerge_state === 'merged'(merged via GitHub UI), show success banner and hide merge button instead of leaving it visibleTest Plan
Relates to #571
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes