Skip to content

fix(web): a refused review shows GitHub's reason - #22382

Open
tiliakoos wants to merge 1 commit into
pingdotgg:mainfrom
tiliakoos:fix/12549-review-refusal-toast
Open

tiliakoos wants to merge 1 commit into
pingdotgg:mainfrom
tiliakoos:fix/12549-review-refusal-toast

Conversation

@tiliakoos

Copy link
Copy Markdown
Contributor

Problem

When GitHub refuses a pull request review, the web client shows only "The review could not be submitted", so the reviewer cannot tell what to fix (#12549). GitHub's reason already reaches the client: submitReview posts through the REST client, whose errors carry GitHub's message and errors[] since #19224, and that text arrives as PullRequestOperationError.detail. The toast in PullRequestReviewForm.tsx never reads it. The mobile review sheet already shows the failure's message.

Change

The failure toast gets a description from readableFailure(squashAtomCommandFailure(result), hint), the same call the label and reviewer pickers use for their refusals. It drops the "Pull request operation … failed:" prefix, falls back to the hint when the host gives nothing readable, and caps the text at 320 characters. The title, the kept draft and the success path are unchanged. Web only: the server already sends the reason.

Scope and approval

Refs #12549. This is item 3 of Julius's triage: "Surface GitHub’s 422. If something still slips through, … show that string in the toast. PullRequestOperationError already has a detail field; the UI just does not use it here." Item 1, refusing anchors outside the hunks, landed in #12583.

It picks up agomesp's #13293, which was closed for missing before/after screenshots of this toast and the kept draft. Its server half, carrying the gh api error body into detail, is no longer needed after #19224.

Verification

  • vp lint and vp fmt --check on the file and the @t3tools/web typecheck are clean. pullRequestDetail.logic.test.ts, which covers readableFailure, passes (140 tests). No new test: the change passes an existing, tested helper's output to the toast.

  • Real app: the dev desktop app from this branch on an empty .t3, signed in through gh, against a throwaway pull request in a private scratch repository. I drafted a line comment on src/greeting.ts:10, pushed a commit that took src/greeting.ts out of the pull request, and submitted the review with that comment still pending. GitHub refused it each time, and the summary and the pending comment ("Review (1)") stayed.

    Before (main's PullRequestReviewForm.tsx, same session and draft):

    main: the toast reads only 'The review could not be submitted'; the summary and one pending comment stay

    This branch, where the toast adds GitHub's reason, "GitHub returned an error: Unprocessable Entity; Path could not be resolved":

    this branch: the toast adds 'GitHub returned an error: Unprocessable Entity; Path could not be resolved'; the summary and one pending comment stay
  • After discarding the stale comment, the same review went through: "Review submitted", the summary cleared, and GitHub lists the review.

  • Not checked: GitHub's Line could not be resolved from the issue, since fix(web): keep review comments inside the pull request's hunks #12583 now refuses that anchor before a comment exists; other hosts (GitLab, Bitbucket, Azure DevOps), whose providers fill detail in their own words; mobile, unchanged.

Claude Opus 5.5 via T3 Code

The review failure toast ignored PullRequestOperationError.detail, which
already carries GitHub's refusal. Show it, as the label and reviewer pickers do.
@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Oct 20, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 3582b8c

Macroscope's review found this PR approvable — This is a focused web bug fix that surfaces an existing host refusal reason in the review-submission error toast while preserving the draft and all submission behavior. It changes one failure-message path, uses existing tested helpers, and introduces no schema, default, security, or deployment changes.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: a6b4ed51-642a-4f72-8817-49ec49bfcd07
📥 Commits

Reviewing files that changed from the base of the PR and between 67c0fc0 and 3582b8c.

📒 Files selected for processing: 1
  • apps/web/src/components/pullRequest/PullRequestReviewForm.tsx

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

The review form now shows a readable host failure reason when review submission fails. The error toast also states that the draft remains available for retry.

Changes: Review submission errors

Layer / File(s) Summary
Failure details and retry message
apps/web/src/components/pullRequest/PullRequestReviewForm.tsx
The form uses failure-normalization helpers and displays a descriptive failure message with the error toast. The message states that the draft is preserved for retry.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~5 minutes

Change: Bug fix

Suggested reviewers: maria-rcks

Merge Risk: ⚪ Minimal · up to 3582b

The updated toast safely presents provider refusal details and preserves retry guidance.

Security architecture risk: 🔵 Low · up to 3582b

Review requests and draft handling remain unchanged. Error text becomes visible to the submitting reviewer, but its display-safety is not fully established for every provider or unexpected failure. No sensitive-data disclosure was demonstrated.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The changed exposure is failure-text presentation in the submitting web client. The diff does not expand request targets, credentials, provider access or submission authority; error detail was already transported to that client.

Security Findings and Attack Paths

  • inferred — The disclosure candidate is not established. Forgejo response-body text can reach the new description, and unexpected client defects can become displayable failures, but no inspected path demonstrated credentials, sensitive local data or another user's data reaching this sink. Provider-wide display guarantees and complete defect propagation remain partially covered.

Trust Boundaries and Controls

  • observed — GitHub supplies structured message and error-field explanations rather than forwarding the complete response body, and uses fixed messages for transport and several operational failures. This is strong counterevidence to treating the intended GitHub refusal display as arbitrary local-error disclosure.
  • observed — The formatter's prefix removal, noise fallback and length cap are presentation controls, not secrecy filters. Forgejo's existing handling preserves non-404 HTTP response text, so the newly visible content is not universally restricted to structured refusal fields.

Hardening Proposals

  • proposed — Consider an explicit display-safe provider error contract, with a generic fallback for unexpected defects and unclassified response bodies, so future consumers do not have to infer whether arbitrary error messages are suitable for display.
Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: showing GitHub's reason when a web review is refused.
Description check Passed The description covers the problem, change, scope and approval, verification results, screenshots, limitations, and the agent used. It is specific to the pull request and meets the template requiremen…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description covers the problem, change, scope and approval, verification results, screenshots, limitations, and the agent used. It is specific to the pull request and meets the template requirements.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant