feat: retrigger check-run using v3 server endpoint #369
Conversation
… chore/disable-all-jobs
… chore/disable-all-jobs
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces modifications to job definitions in the Trigger.dev configuration and the submissions API server route. The changes primarily involve disabling specific jobs by setting their Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
… chore/disable-all-jobs
|
Needs https://github.com/holdex/pr-time-tracker-webhooks/pull/455 to be merged first |
|
@georgeciubotaru please let me know if there is any more place where it triggers |
|
after this gets merged, I'll remove all existing webhooks to the prod cloud v2 server, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/routes/api/submissions/+server.ts (1)
170-193: Consider enhancing error handling and request reliability.While the implementation is functional, consider these improvements:
- Add specific error handling for different failure scenarios (network issues, authentication errors, etc.)
- Specify a timeout for the axios request
- Implement a retry mechanism for transient failures
Here's a suggested implementation:
async function triggerRequestCheckRun(data: { org: string; repoName: string; senderId: number; senderLogin: string; prNumber: number; }) { try { const url = TRIGGER_SERVER_URL; const secret = TRIGGER_SERVER_SECRET; if (!url || !secret) throw new Error('Trigger server not configured'); const res = await axios.post(`${url}/api/submission-event`, data, { headers: { 'x-trigger-server-secret': secret - } + }, + timeout: 5000, // 5 second timeout + validateStatus: status => status === 200 // Only 200 is considered success }); - if (res.status !== 200) throw new Error(res.data.message); } catch (e) { - throw new Error((e as any)?.response?.data || 'Failed to trigger check run'); + if (axios.isAxiosError(e)) { + if (e.code === 'ECONNABORTED') { + throw new Error('Request timed out'); + } + if (e.response) { + throw new Error(`Server error: ${e.response.data?.message || e.response.statusText}`); + } + if (e.request) { + throw new Error('No response received from server'); + } + } + throw new Error('Failed to trigger check run: ' + e.message); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/server/trigger-dev/jobs/index.ts(3 hunks)src/routes/api/submissions/+server.ts(4 hunks)
🔇 Additional comments (5)
src/routes/api/submissions/+server.ts (2)
2-2: LGTM! Necessary imports added.The addition of axios and environment variables from private env is appropriate for the new functionality.
Also applies to: 5-5
66-72: LGTM! Consistent parameter passing in both handlers.The replacement of
checkRunFromEventwithtriggerRequestCheckRunis implemented consistently in both POST and PATCH handlers, with appropriate parameter mapping from their respective contexts.Also applies to: 142-148
src/lib/server/trigger-dev/jobs/index.ts (3)
144-144: LGTM! Disabled non-essential jobs.Disabling discord-send-message and github-create-bug-report-issue jobs is appropriate as they are not critical for the check-run functionality.
Also applies to: 166-166
111-111: LGTM! Appropriate concurrency limit.Setting concurrencyLimit to 1 for check-run related jobs is appropriate to prevent potential race conditions when handling check runs.
110-110: Verify impact of disabling custom-event-streaming job.The job is being disabled, but based on the PR objectives and comments, this is the only instance where the custom-event job is triggered. Please confirm if this is intentional.
Run this script to verify usage:
✅ Verification successful
Disabling custom-event-streaming job appears intentional and safe
The custom events are used for auxiliary monitoring and don't affect core GitHub integration functionality. All critical event handlers (issues, PRs, reviews, check runs) remain enabled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for custom event triggers in the codebase echo "Searching for custom event triggers..." rg -l "custom_event" --type ts --type js echo "Searching for sendEvent calls..." rg -l "sendEvent.*custom.*event" --type ts --type jsLength of output: 334
Script:
#!/bin/bash # Check content of files and broader event patterns echo "Content of utils.ts:" rg -A 5 -B 5 "custom_event" src/lib/server/trigger-dev/utils.ts echo -e "\nContent of jobs/index.ts:" rg -A 5 -B 5 "custom_event" src/lib/server/trigger-dev/jobs/index.ts echo -e "\nBroader search for event handling:" rg "event" src/lib/server/trigger-dev/utils.ts src/lib/server/trigger-dev/jobs/index.tsLength of output: 2759
|
@teodorus-nathaniel merge the PR after clarifiying with the previous one https://github.com/holdex/pr-time-tracker-webhooks/pull/455 |
|
@georgeciubotaru action done in previous pr, Ill merge this |
resolves https://github.com/holdex/pr-time-tracker-webhooks/issues/454
Summary by CodeRabbit
Configuration Changes
API Improvements