Test/discord testing#470
Conversation
📝 WalkthroughWalkthroughUpdates Discord webhook response handling to parse JSON conditionally based on content-type, read error text upfront, and relax secret validation to only fail on specific event types ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/discord.yaml:
- Around line 212-223: The set strictEvents currently includes
"workflow_dispatch" which is dead here because getPullRequest()/pr-check runs
earlier; remove "workflow_dispatch" from the strictEvents Set (where
strictEvents is defined and used with context.eventName) so only events that can
reach this branch (e.g., "pull_request_target") will call core.setFailed, or
alternatively move this webhook-missing early-fail logic before the
getPullRequest()/pr check if your intent is to fail on manual workflow_dispatch
runs—pick one approach and apply it to the block that defines/uses strictEvents
and the subsequent core.setFailed/core.warning calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 538a9943-217f-45ea-be67-87530f6a2206
📒 Files selected for processing (2)
.github/workflows/discord.yamlREADME.md
| if (!webhookUrl) { | ||
| core.setFailed( | ||
| "Missing Discord webhook secret. Set either DISCORD_WEBHOOK_URL or DISCORD_PR_FORUM_WEBHOOK in repository secrets, or pass it explicitly if using reusable workflows." | ||
| ); | ||
| const strictEvents = new Set(["pull_request_target", "workflow_dispatch"]); | ||
| const msg = | ||
| `Discord sync skipped: webhook secret unavailable for event '${context.eventName}'. ` + | ||
| "Set either DISCORD_WEBHOOK_URL or DISCORD_PR_FORUM_WEBHOOK in repository secrets."; | ||
| if (strictEvents.has(context.eventName)) { | ||
| core.setFailed(msg); | ||
| } else { | ||
| core.warning(msg); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
nit: workflow_dispatch in strictEvents is effectively dead code
getPullRequest() only returns a PR for pull_request_target, pull_request_review, and issue_comment. For workflow_dispatch it returns null, so we bail at the if (!pr) check on line 207-210 before ever reaching this webhook-missing branch. So in practice only pull_request_target can ever trigger setFailed here — workflow_dispatch will never hit it.
not a bug, just kinda misleading. either drop workflow_dispatch from the set, or (if the intent was "fail loudly on manual runs with misconfig") move a similar check earlier so it actually fires. totally fine to leave as-is too, just flagging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/discord.yaml around lines 212 - 223, The set strictEvents
currently includes "workflow_dispatch" which is dead here because
getPullRequest()/pr-check runs earlier; remove "workflow_dispatch" from the
strictEvents Set (where strictEvents is defined and used with context.eventName)
so only events that can reach this branch (e.g., "pull_request_target") will
call core.setFailed, or alternatively move this webhook-missing early-fail logic
before the getPullRequest()/pr check if your intent is to fail on manual
workflow_dispatch runs—pick one approach and apply it to the block that
defines/uses strictEvents and the subsequent core.setFailed/core.warning calls.
1 participant
Pull Request Template
Description
Motivation
Type of Change
Related Issue(s)
Screenshots / Video
Screenshot (if applicable):
Video (if applicable):
Testing
Checklist
Thank you for contributing!
Summary by CodeRabbit