feat: Calculate actual average response time from comment data#237
feat: Calculate actual average response time from comment data#237EdiWeeks wants to merge 4 commits into
Conversation
Replaces workload-based estimate with actual first-response time calculated from work item comments. Fetches comments for tickets from the last 30 days in parallel batches of 10, with a 5-minute TTL cache. Falls back to workload estimate when no data available. Closes #179 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR implements actual average response time calculation based on work item comments, replacing the previous workload-based estimation. The implementation fetches comments for tickets from the last 30 days, identifies the first internal (non-requester) response, and calculates actual response times. To mitigate performance impact, it uses batched parallel fetching (10 concurrent requests) and a 5-minute TTL cache.
Changes:
- Added comment-based response time calculation with 30-day lookback window
- Implemented module-level caching with 5-minute TTL to reduce API load
- Added parallel comment fetching in batches of 10 using Promise.allSettled
- Retained workload-based estimation as fallback when no comment data available
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (authorEmail === requesterEmail) continue; | ||
|
|
||
| const responseTimeMs = comment.createdAt.getTime() - ticket.createdAt.getTime(); | ||
| if (responseTimeMs <= 0) continue; |
There was a problem hiding this comment.
The check for responseTimeMs <= 0 at line 290 might filter out valid same-millisecond responses. While unlikely, if a comment is created in the same millisecond as the ticket (especially in automated systems), it would be incorrectly excluded. The condition should be responseTimeMs < 0 to only exclude invalid negative values while allowing zero (immediate response).
| if (responseTimeMs <= 0) continue; | |
| if (responseTimeMs < 0) continue; |
There was a problem hiding this comment.
A 0ms response is almost certainly an automated system comment, not a genuine human response. Keeping <= 0 to filter those out.
There was a problem hiding this comment.
I don't think we need an IF here ... even a system automated response won't be 0ms. Suggest we keep it symple for now and not worry about agentic responses (that used to ignore automated initial "thank you for you message" responses, but will would look to implement an actual useful agentic response).
There was a problem hiding this comment.
Agreed — removed the check entirely in d6822d7. Keeping it simple.
- Extract shared formatDuration() helper to reduce duplication - Add error logging to catch block in getFirstResponseTimeMs() - Guard requester email check to handle empty string edge case - Cap ticket processing at 100 most recent to prevent excessive API calls Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Keep response time cache from PR branch and request parameter from main. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Per review feedback — even automated responses won't be 0ms, so the guard is unnecessary. Keep it simple. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@BenGWeeks please approve |
Summary
Promise.allSettledTest plan
Closes #179
🤖 Generated with Claude Code