Refactor: Extract duplicate expired entity cleanup orchestration#14292
Refactor: Extract duplicate expired entity cleanup orchestration#14292
Conversation
…elper - Created expired_entity_main_flow.cjs with executeExpiredEntityCleanup() - Refactored close_expired_discussions.cjs to use shared helper (reduced from 220 to 157 lines) - Refactored close_expired_issues.cjs to use shared helper (reduced from 140 to 78 lines) - Refactored close_expired_pull_requests.cjs to use shared helper (reduced from 139 to 77 lines) - Added comprehensive tests for all refactored files - Total reduction: ~250+ duplicate lines eliminated Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🔍 PR Triage ResultsCategory: refactor | Risk: medium | Priority: 56/100 Scores Breakdown
📋 Recommended Action: batch_reviewHigh-quality refactor addressing code duplication. Groups with PR #14289 for batch workflow/code improvements review. Batch ID: batch-refactor-001 (grouped with PRs #14289) Triaged by PR Triage Agent on 2026-02-07
|
🔍 PR Triage ResultsCategory: refactor | Risk: medium | Priority: 56/100 Scores Breakdown
📋 Recommended Action: batch_reviewThis code cleanup refactoring is part of the code deduplication batch ( Next Steps: Fix CI issues, then review for merge. Consider grouping with other deduplication PRs if they emerge. Triaged by PR Triage Agent on 2026-02-07
|
There was a problem hiding this comment.
Pull request overview
Refactors the three “close expired *” cleanup scripts by extracting the shared orchestration (search → categorize → process → summarize) into a reusable helper, reducing duplication and adding/expanding test coverage around the shared flow and issue cleanup behavior.
Changes:
- Added a shared orchestrator
executeExpiredEntityCleanup()to centralize the expired-entity cleanup flow. - Updated discussions/issues/pull-requests cleanup scripts to delegate orchestration to the shared helper and keep only entity-specific close/comment logic.
- Added new integration-style tests for the shared flow and introduced comprehensive tests for
close_expired_issues.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/expired_entity_main_flow.cjs | New shared orchestration helper to standardize expired entity cleanup flow. |
| actions/setup/js/close_expired_discussions.cjs | Refactored to use the shared orchestration helper with discussion-specific behavior (dedupe + skipped section). |
| actions/setup/js/close_expired_issues.cjs | Refactored to use the shared orchestration helper with issue-specific close/comment behavior. |
| actions/setup/js/close_expired_pull_requests.cjs | Refactored to use the shared orchestration helper with PR-specific close/comment behavior. |
| actions/setup/js/expired_entity_main_flow.test.cjs | New integration tests for the shared orchestration helper. |
| actions/setup/js/close_expired_issues.test.cjs | New comprehensive test suite for closing expired issues (previously missing). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @typedef {Object} EntityFlowConfig | ||
| * @property {string} entityType - Entity type name for logging (e.g., "issues", "pull requests", "discussions") | ||
| * @property {string} graphqlField - GraphQL field name (e.g., "issues", "pullRequests", "discussions") | ||
| * @property {string} resultKey - Key to use in return object (e.g., "issues", "pullRequests", "discussions") |
There was a problem hiding this comment.
EntityFlowConfig.resultKey is documented as "Key to use in return object", but this flow doesn’t return an object and only forwards resultKey to searchEntitiesWithExpiration() (which doesn’t appear to use it). Consider removing resultKey from this typedef/callers, or updating the JSDoc to reflect its actual purpose (or that it’s unused).
| * @property {string} resultKey - Key to use in return object (e.g., "issues", "pullRequests", "discussions") | |
| * @property {string} resultKey - Key forwarded to searchEntitiesWithExpiration (reserved for its internal/result handling) |
| core.info(`Found ${entitiesWithExpiration.length} ${config.entityType.slice(0, -1)}(s) with expiration markers`); | ||
|
|
There was a problem hiding this comment.
Deriving the singular form via config.entityType.slice(0, -1) assumes entityType is a plural ending in "s". If a caller ever passes a non-"...s" plural (or an already-singular string), the output will be wrong (e.g., "issue" → "issu"). Consider making the singular form explicit in config (or derive from entityLabel.toLowerCase()), and/or validate entityType before slicing so the contract is explicit.
Three entity cleanup scripts (
close_expired_discussions,close_expired_issues,close_expired_pull_requests) duplicated ~250 lines of identical orchestration logic (search → categorize → process → summarize).Changes
New shared helper:
expired_entity_main_flow.cjsexecuteExpiredEntityCleanup()processEntitycallbackRefactored entity files
close_expired_discussions.cjs: 220 → 156 lines (-29%)close_expired_issues.cjs: 140 → 77 lines (-45%)close_expired_pull_requests.cjs: 139 → 76 lines (-45%)Test coverage
expired_entity_main_flow.test.cjs(4 integration tests)close_expired_issues.test.cjs(5 comprehensive tests, was missing)Example
Before (duplicated in each file):
After:
Impact
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.