Skip to content

React with emoji for better UX on explicit review ask - #62

Open
masih wants to merge 2 commits into
mainfrom
masih/ai-on-demand-review-emoji
Open

React with emoji for better UX on explicit review ask#62
masih wants to merge 2 commits into
mainfrom
masih/ai-on-demand-review-emoji

Conversation

@masih

@masih masih commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

When asked for explicit AI review, react with emojis to let the user know it is happening.

When asked for explicit AI review, react with emojis to let the user know it is happening.
@masih
masih marked this pull request as ready for review August 17, 2026 11:27
@cursor

cursor Bot commented Aug 17, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Workflow-only UX around reactions; scoped to authorized explicit review commands and uses existing app/token patterns with fail-soft warnings on GraphQL errors.

Overview
Explicit @seidroid review requests now get best-effort GitHub reactions so requesters see progress without polling the Actions tab.

Preflight gains pull-requests: write and, after an authorized explicit request, adds a 👀 reaction on the triggering comment or review via GraphQL and passes the subject node_id through as reaction_subject_id.

A new complete_review_reaction job runs when that ID is set (even if review jobs fail). It removes the 👀 reaction and adds 👍 only when claude_review succeeds; failures still clear the in-progress reaction.

The ai-review README table documents this behavior for callers.

Reviewed by Cursor Bugbot for commit 3ce196d. Bugbot is set up for automated code reviews on this repo. Configure here.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit d556797. Configure here.

Comment thread .github/workflows/ai-review.yml Outdated
Comment thread .github/workflows/ai-review.yml Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adds 👀/👍 reaction feedback for explicit @seidroid review requests, but the reaction logic has no error handling and signals success unconditionally: an unguarded addReaction in preflight can skip the entire review pipeline, and the completion job posts 👍 even when the review failed or was cancelled.

Findings: 2 blocking | 3 non-blocking | 3 posted inline

Blockers

  • None at the file/PR level.
  • 2 blocking issue(s) flagged inline on specific lines.

Non-blocking

  • The file's header comment (lines 3–12) enumerates the pipeline jobs (preflight, codex_review, cursor_review, claude_review) and is not updated for the new complete_review_reaction job; .github/seidroid/ai-review/README.md also doesn't mention the new 👀/👍 feedback behaviour or that it only applies to explicit @seidroid review requests.
  • preflight is escalated from pull-requests: read to write solely to post the 👀 reaction, which weakens the least-privilege split this workflow's header comment emphasizes. Consider keeping preflight read-only and emitting only reaction_subject_id, with a tiny separate start_review_reaction job (mirroring complete_review_reaction) holding the write scope.
  • 1 suggestion(s)/nit(s) flagged inline on specific lines.

Comment thread .github/workflows/ai-review.yml Outdated
return;
}

await github.graphql(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[blocker] This mutation is unguarded, so a failed reaction kills the whole review. If addReaction throws (403 on the reaction scope, GraphQL transient/rate-limit error, non-reactable subject), the resolve step fails → preflight fails → claude_review is skipped by its needs.preflight.result == 'success' gate. A purely cosmetic emoji should never block the review pipeline.

Wrap it in try/catch and core.warning on failure (only setting reaction_subject_id when the reaction actually landed), matching the tolerant style used in ai-assistant.yml (gh api ... || true).

Worth verifying the permission path too: for issue_comment events the subject is an issue comment, and callers are only documented to grant pull-requests: write (README lines 48–52). If issues: write turns out to be required for that subject type, every explicit review request in downstream repos would fail here rather than degrade.

complete_review_reaction:
name: Complete review reaction
needs: [preflight, codex_review, cursor_review, claude_review]
if: ${{ always() && needs.preflight.outputs.reaction_subject_id != '' }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[blocker] The 👍 is posted unconditionally, so a failed review is reported as a successful one. The condition only checks always() and that a subject id exists — it ignores the results of the jobs it depends on. Consequences:

  • claude_review fails (or preflight fails after resolve set the output, so no review ever ran): 👀 is cleared and 👍 added anyway, while no AI Review check run was created.
  • The run is cancelled (a push during a comment-triggered review cancels it, since cancel-in-progress is true for pull_request events): always() still runs on cancellation → 👍 for a review that never finished.

Gate the success reaction on needs.claude_review.result == 'success' and, for other outcomes, either just clear 👀 (what ai-assistant.yml:236-242 does) or add a distinct reaction such as CONFUSED/THUMBS_DOWN so a broken pipeline is visible to the requester.

Comment thread .github/workflows/ai-review.yml Outdated
github-token: ${{ steps.app-token.outputs.token || github.token }}
script: |
const subjectId = process.env.SUBJECT_ID;
await github.graphql(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] Both mutations are unguarded, so any failure here red-Xes an otherwise successful review run. In particular, if the 👀 was already removed (manually, or the app identity differs because preflight used the app token while this job fell back to github.token), removeReaction errors and the 👍 is never added at all. Wrap each call in try/catch with core.warning, and consider adding the completion reaction before removing 👀 so the visible end state doesn't depend on the cleanup succeeding.

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adds a best-effort 👀/👍 reaction lifecycle for explicit @seidroid review requests; the design is sound (gated behind authorization, all GraphQL calls wrapped in try/catch so failures only warn). Findings are non-blocking: a likely missing issues: write scope on the GITHUB_TOKEN fallback path, a stale-👀 case when a run is cancelled, and no signal to the requester when the review fails.

Findings: 0 blocking | 5 non-blocking | 4 posted inline

Blockers

  • None at the file/PR level.

Non-blocking

  • Behavior is untested/unverifiable in CI (no workflow lint or dry-run for the new job). Worth a manual smoke test of all three explicit-request paths — issue_comment, pull_request_review_comment, and pull_request_review — since only the last uses review.node_id and the first two use comment.node_id.
  • 4 suggestion(s)/nit(s) flagged inline on specific lines.

permissions:
contents: read
pull-requests: read
pull-requests: write

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] The most common trigger path is issue_comment (a @seidroid review comment on the PR conversation), whose node_id is an IssueComment. Reactions on issue comments are gated by the Issues permission, not Pull requests — this repo's own ai-assistant.yml declares issues: write # reply on the PR conversation timeline for exactly that (.github/workflows/ai-assistant.yml:94) and reacts via repos/.../issues/comments/{id}/reactions.

So when app credentials are absent and github.token is used as the fallback, addReaction here (and removeReaction in complete_review_reaction, which grants the same scope at line 1059) will 403 and only emit a warning — reactions silently never appear. Note this is a two-part fix: because a reusable workflow cannot elevate beyond the caller's grant, adding issues: write to these job blocks also requires adding it to .github/workflows/ai-review-self.yml and to the caller snippet in .github/seidroid/ai-review/README.md, otherwise the run fails outright for callers that don't grant it.

complete_review_reaction:
name: Complete review reaction
needs: [preflight, codex_review, cursor_review, claude_review]
if: ${{ always() && needs.preflight.outputs.reaction_subject_id != '' }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] always() covers failed and skipped upstream jobs, but not run cancellation: cancel-in-progress is true for pull_request events (line 105), so a push while a comment-triggered review is in flight cancels this run, and a queued job in a cancelled run does not start regardless of always(). The 👀 then stays on the comment forever, which reads as "still working" indefinitely — and pushing a fix mid-review is a common flow.

Cheapest mitigation: have the preflight reaction step also remove any pre-existing EYES reaction from the same subject before adding a fresh one, so a subsequent run self-heals the stale state.

uses: actions/github-script@v9
env:
SUBJECT_ID: ${{ needs.preflight.outputs.reaction_subject_id }}
REVIEW_SUCCEEDED: ${{ needs.claude_review.result == 'success' }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] When claude_review fails or is cancelled, the 👀 is cleared and nothing replaces it, so a failed review is indistinguishable from "the request was never picked up" — the requester has no reason to look at the Actions tab. Consider a distinct terminal reaction on the non-success branch (e.g. CONFUSED or THUMBS_DOWN) so the outcome is always visible on the comment.

- name: Generate GitHub App token
id: app-token
if: steps.creds.outputs.present == 'true'
continue-on-error: true

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] continue-on-error: true here silently changes the acting identity: if token generation fails, github-token falls back to github.token, and GraphQL removeReaction only removes the viewer's own reaction — so the app's 👀 cannot be cleared and the 👍 is posted by a different account. Preflight's equivalent step (line 173) has no continue-on-error, so this mismatch is only reachable via a transient failure; letting the step fail hard would at least make the leftover reaction traceable to a red job rather than a warning.

@seidroid
seidroid Bot dismissed github-actions[bot]’s stale review August 17, 2026 13:10

Superseded: latest AI review found no blocking issues.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant