Refactor expired-entity cleanup scripts to share expiration processing #13420
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codex
AI
changed the title
Contributor
🔍 PR Triage ResultsCategory: refactor | Risk: medium | Priority: 52/100 Scores Breakdown
📋 Recommended Action: batch_reviewThis is a well-structured refactoring PR that addresses code duplication. The changes are moderate in scope and improve maintainability. Once CI passes and draft status is removed, it should be reviewed alongside other JavaScript refactoring PRs. Batch ID: batch-refactor-001 - JavaScript refactoring PRs for code cleanup Triaged by PR Triage Agent on 2026-02-03
|
pelikhan
approved these changes
Feb 3, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors expired-entity cleanup scripts to eliminate code duplication and centralize shared logic for expiration processing across issues, pull requests, and discussions.
Changes:
- Introduced
expired_entity_cleanup_helpers.cjsto centralize expiration date parsing, categorization, rate-limited processing, and summary generation - Updated
close_expired_issues.cjs,close_expired_pull_requests.cjs, andclose_expired_discussions.cjsto use the new shared helper functions - Preserved entity-specific behaviors (e.g., discussion duplicate comment detection) via callback-based architecture
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
expired_entity_cleanup_helpers.cjs |
New shared helper module containing expiration categorization, processing loop, and summary generation functions |
close_expired_pull_requests.cjs |
Refactored to delegate expiration logic to shared helper, reducing from 279 to 139 lines |
close_expired_issues.cjs |
Refactored to delegate expiration logic to shared helper, reducing from 279 to 140 lines |
close_expired_discussions.cjs |
Refactored to delegate expiration logic to shared helper while preserving skip/duplicate handling, reducing from 340 to 220 lines |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
251
to
+176
| if (hasComment) { | ||
| core.warning(` Discussion #${discussion.number} already has an expiration comment, skipping to avoid duplicate`); | ||
| skippedDiscussions.push({ | ||
| number: discussion.number, | ||
| url: discussion.url, | ||
| title: discussion.title, | ||
| }); | ||
| skippedCount++; | ||
|
|
||
| // Still try to close it if it's somehow still open | ||
| core.info(` Attempting to close discussion #${discussion.number} without adding another comment`); | ||
| await closeDiscussionAsOutdated(github, discussion.id); | ||
| core.info(` ✓ Discussion closed successfully`); | ||
|
|
||
| closedDiscussions.push({ | ||
| number: discussion.number, | ||
| url: discussion.url, | ||
| title: discussion.title, | ||
| }); | ||
| closedCount++; | ||
| } else { | ||
| const closingMessage = `This discussion was automatically closed because it expired on ${discussion.expirationDate.toISOString()}.\n\n<!-- gh-aw-closed -->`; | ||
|
|
||
| // Add comment first | ||
| core.info(` Adding closing comment to discussion #${discussion.number}`); | ||
| await addDiscussionComment(github, discussion.id, closingMessage); | ||
| core.info(` ✓ Comment added successfully`); | ||
|
|
||
| // Then close the discussion as outdated | ||
| core.info(` Closing discussion #${discussion.number} as outdated`); | ||
| await closeDiscussionAsOutdated(github, discussion.id); | ||
| core.info(` ✓ Discussion closed successfully`); | ||
|
|
||
| closedDiscussions.push({ | ||
| number: discussion.number, | ||
| url: discussion.url, | ||
| title: discussion.title, | ||
| }); | ||
|
|
||
| closedCount++; | ||
| return { | ||
| status: "skipped", | ||
| record: { | ||
| number: discussion.number, | ||
| url: discussion.url, | ||
| title: discussion.title, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
The refactored code changes the behavior when a discussion already has an expiration comment. In the original code, when a discussion had an existing comment, it would:
- Add the discussion to the skipped list
- Close the discussion
- Also add it to the closed list
This meant the discussion appeared in both lists (skipped for having a comment, but still counted as closed).
The refactored code returns with status "skipped" immediately after closing (line 170), which means the discussion is only counted as "skipped" and not as "closed". This changes the summary statistics and could be misleading, as the discussion is actually being closed but not reflected in the "Successfully closed" count.
Comment on lines
+1
to
+250
| // @ts-check | ||
| // <reference types="@actions/github-script" /> | ||
|
|
||
| const { extractExpirationDate } = require("./ephemerals.cjs"); | ||
| const { getErrorMessage } = require("./error_helpers.cjs"); | ||
|
|
||
| const DEFAULT_MAX_UPDATES_PER_RUN = 100; | ||
| const DEFAULT_GRAPHQL_DELAY_MS = 500; | ||
|
|
||
| /** | ||
| * Delay execution for a specified number of milliseconds | ||
| * @param {number} ms - Milliseconds to delay | ||
| * @returns {Promise<void>} | ||
| */ | ||
| function delay(ms) { | ||
| return new Promise(resolve => setTimeout(resolve, ms)); | ||
| } | ||
|
|
||
| /** | ||
| * Validate entity creation date | ||
| * @param {string} createdAt - ISO 8601 creation date | ||
| * @returns {boolean} True if valid | ||
| */ | ||
| function validateCreationDate(createdAt) { | ||
| const creationDate = new Date(createdAt); | ||
| return !isNaN(creationDate.getTime()); | ||
| } | ||
|
|
||
| /** | ||
| * Categorize entities by expiration state and log status | ||
| * @param {Array<{number: number, title: string, url: string, body: string, createdAt: string}>} entities | ||
| * @param {{entityLabel: string}} options | ||
| * @returns {{expired: Array<any>, notExpired: Array<any>, now: Date}} | ||
| */ | ||
| function categorizeByExpiration(entities, { entityLabel }) { | ||
| const now = new Date(); | ||
| core.info(`Current date/time: ${now.toISOString()}`); | ||
|
|
||
| const expired = []; | ||
| const notExpired = []; | ||
|
|
||
| for (const entity of entities) { | ||
| core.info(`Processing ${entityLabel} #${entity.number}: ${entity.title}`); | ||
|
|
||
| if (!validateCreationDate(entity.createdAt)) { | ||
| core.warning(` ${entityLabel} #${entity.number} has invalid creation date: ${entity.createdAt}, skipping`); | ||
| continue; | ||
| } | ||
| core.info(` Creation date: ${entity.createdAt}`); | ||
|
|
||
| const expirationDate = extractExpirationDate(entity.body); | ||
| if (!expirationDate) { | ||
| core.warning(` ${entityLabel} #${entity.number} has invalid expiration date format, skipping`); | ||
| continue; | ||
| } | ||
| core.info(` Expiration date: ${expirationDate.toISOString()}`); | ||
|
|
||
| const isExpired = now >= expirationDate; | ||
| const timeDiff = expirationDate.getTime() - now.getTime(); | ||
| const daysUntilExpiration = Math.floor(timeDiff / (1000 * 60 * 60 * 24)); | ||
| const hoursUntilExpiration = Math.floor(timeDiff / (1000 * 60 * 60)); | ||
|
|
||
| if (isExpired) { | ||
| const daysSinceExpiration = Math.abs(daysUntilExpiration); | ||
| const hoursSinceExpiration = Math.abs(hoursUntilExpiration); | ||
| core.info(` ✓ ${entityLabel} #${entity.number} is EXPIRED (expired ${daysSinceExpiration} days, ${hoursSinceExpiration % 24} hours ago)`); | ||
| expired.push({ | ||
| ...entity, | ||
| expirationDate, | ||
| }); | ||
| } else { | ||
| core.info(` ✗ ${entityLabel} #${entity.number} is NOT expired (expires in ${daysUntilExpiration} days, ${hoursUntilExpiration % 24} hours)`); | ||
| notExpired.push({ | ||
| ...entity, | ||
| expirationDate, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| core.info(`Expiration check complete: ${expired.length} expired, ${notExpired.length} not yet expired`); | ||
| return { expired, notExpired, now }; | ||
| } | ||
|
|
||
| /** | ||
| * Process expired entities with per-entity handler and rate limiting | ||
| * @param {Array<any>} expiredEntities | ||
| * @param {{ | ||
| * entityLabel: string, | ||
| * maxPerRun?: number, | ||
| * delayMs?: number, | ||
| * processEntity: (entity: any) => Promise<{status: "closed" | "skipped", record: any}> | ||
| * }} options | ||
| * @returns {Promise<{closed: Array<any>, skipped: Array<any>, failed: Array<any>}>} | ||
| */ | ||
| async function processExpiredEntities(expiredEntities, { entityLabel, maxPerRun = DEFAULT_MAX_UPDATES_PER_RUN, delayMs = DEFAULT_GRAPHQL_DELAY_MS, processEntity }) { | ||
| const entitiesToProcess = expiredEntities.slice(0, maxPerRun); | ||
|
|
||
| if (expiredEntities.length > maxPerRun) { | ||
| core.warning(`Found ${expiredEntities.length} expired ${entityLabel.toLowerCase()}s, but only closing the first ${maxPerRun}`); | ||
| core.info(`Remaining ${expiredEntities.length - maxPerRun} expired ${entityLabel.toLowerCase()}s will be closed in the next run`); | ||
| } | ||
|
|
||
| core.info(`Preparing to close ${entitiesToProcess.length} ${entityLabel.toLowerCase()}(s)`); | ||
|
|
||
| const closed = []; | ||
| const failed = []; | ||
| const skipped = []; | ||
|
|
||
| for (let i = 0; i < entitiesToProcess.length; i++) { | ||
| const entity = entitiesToProcess[i]; | ||
| core.info(`[${i + 1}/${entitiesToProcess.length}] Processing ${entityLabel.toLowerCase()} #${entity.number}: ${entity.url}`); | ||
|
|
||
| try { | ||
| const result = await processEntity(entity); | ||
|
|
||
| if (result.status === "skipped") { | ||
| skipped.push(result.record); | ||
| } else { | ||
| closed.push(result.record); | ||
| } | ||
|
|
||
| core.info(`✓ Successfully processed ${entityLabel.toLowerCase()} #${entity.number}: ${entity.url}`); | ||
| } catch (error) { | ||
| core.error(`✗ Failed to close ${entityLabel.toLowerCase()} #${entity.number}: ${getErrorMessage(error)}`); | ||
| core.error(` Error details: ${JSON.stringify(error, null, 2)}`); | ||
| failed.push({ | ||
| number: entity.number, | ||
| url: entity.url, | ||
| title: entity.title, | ||
| error: getErrorMessage(error), | ||
| }); | ||
| } | ||
|
|
||
| if (i < entitiesToProcess.length - 1) { | ||
| core.info(` Waiting ${delayMs}ms before next operation...`); | ||
| await delay(delayMs); | ||
| } | ||
| } | ||
|
|
||
| return { closed, skipped, failed }; | ||
| } | ||
|
|
||
| /** | ||
| * Build not-yet-expired list section | ||
| * @param {Array<any>} notExpiredEntities | ||
| * @param {Date} now | ||
| * @param {string} entityLabel | ||
| * @returns {string} | ||
| */ | ||
| function buildNotExpiredSection(notExpiredEntities, now, entityLabel) { | ||
| if (notExpiredEntities.length === 0) { | ||
| return ""; | ||
| } | ||
|
|
||
| let section = `### Not Yet Expired\n\n`; | ||
|
|
||
| const list = notExpiredEntities.length > 10 ? notExpiredEntities.slice(0, 10) : notExpiredEntities; | ||
| if (notExpiredEntities.length > 10) { | ||
| section += `${notExpiredEntities.length} ${entityLabel.toLowerCase()}(s) not yet expired (showing first 10):\n\n`; | ||
| } | ||
|
|
||
| for (const entity of list) { | ||
| const timeDiff = entity.expirationDate.getTime() - now.getTime(); | ||
| const days = Math.floor(timeDiff / (1000 * 60 * 60 * 24)); | ||
| const hours = Math.floor(timeDiff / (1000 * 60 * 60)) % 24; | ||
| section += `- ${entityLabel} #${entity.number}: [${entity.title}](${entity.url}) - Expires in ${days}d ${hours}h\n`; | ||
| } | ||
|
|
||
| return section; | ||
| } | ||
|
|
||
| /** | ||
| * Build standardized cleanup summary content | ||
| * @param {{ | ||
| * heading: string, | ||
| * entityLabel: string, | ||
| * searchStats: {totalScanned: number, pageCount: number}, | ||
| * withExpirationCount: number, | ||
| * expired: Array<any>, | ||
| * notExpired: Array<any>, | ||
| * closed: Array<any>, | ||
| * failed: Array<any>, | ||
| * skipped?: Array<any>, | ||
| * maxPerRun: number, | ||
| * includeSkippedHeading?: boolean, | ||
| * now?: Date | ||
| * }} params | ||
| * @returns {string} | ||
| */ | ||
| function buildExpirationSummary(params) { | ||
| const { heading, entityLabel, searchStats, withExpirationCount, expired, notExpired, closed, failed, skipped = [], maxPerRun, includeSkippedHeading = false, now = new Date() } = params; | ||
|
|
||
| let summaryContent = `## ${heading}\n\n`; | ||
| summaryContent += `**Scan Summary**\n`; | ||
| summaryContent += `- Scanned: ${searchStats.totalScanned} ${entityLabel.toLowerCase()}s across ${searchStats.pageCount} page(s)\n`; | ||
| summaryContent += `- With expiration markers: ${withExpirationCount} ${entityLabel.toLowerCase()}(s)\n`; | ||
| summaryContent += `- Expired: ${expired.length} ${entityLabel.toLowerCase()}(s)\n`; | ||
| summaryContent += `- Not yet expired: ${notExpired.length} ${entityLabel.toLowerCase()}(s)\n\n`; | ||
|
|
||
| summaryContent += `**Closing Summary**\n`; | ||
| summaryContent += `- Successfully closed: ${closed.length} ${entityLabel.toLowerCase()}(s)\n`; | ||
| if (includeSkippedHeading && skipped.length > 0) { | ||
| summaryContent += `- Skipped (already had comment): ${skipped.length} ${entityLabel.toLowerCase()}(s)\n`; | ||
| } | ||
| if (failed.length > 0) { | ||
| summaryContent += `- Failed to close: ${failed.length} ${entityLabel.toLowerCase()}(s)\n`; | ||
| } | ||
| if (expired.length > maxPerRun) { | ||
| summaryContent += `- Remaining for next run: ${expired.length - maxPerRun} ${entityLabel.toLowerCase()}(s)\n`; | ||
| } | ||
| summaryContent += `\n`; | ||
|
|
||
| if (closed.length > 0) { | ||
| summaryContent += `### Successfully Closed ${entityLabel}s\n\n`; | ||
| for (const entity of closed) { | ||
| summaryContent += `- ${entityLabel} #${entity.number}: [${entity.title}](${entity.url})\n`; | ||
| } | ||
| summaryContent += `\n`; | ||
| } | ||
|
|
||
| if (includeSkippedHeading && skipped.length > 0) { | ||
| summaryContent += `### Skipped (Already Had Comment)\n\n`; | ||
| for (const entity of skipped) { | ||
| summaryContent += `- ${entityLabel} #${entity.number}: [${entity.title}](${entity.url})\n`; | ||
| } | ||
| summaryContent += `\n`; | ||
| } | ||
|
|
||
| if (failed.length > 0) { | ||
| summaryContent += `### Failed to Close\n\n`; | ||
| for (const entity of failed) { | ||
| summaryContent += `- ${entityLabel} #${entity.number}: [${entity.title}](${entity.url}) - Error: ${entity.error}\n`; | ||
| } | ||
| summaryContent += `\n`; | ||
| } | ||
|
|
||
| summaryContent += buildNotExpiredSection(notExpired, now, entityLabel); | ||
|
|
||
| return summaryContent; | ||
| } | ||
|
|
||
| module.exports = { | ||
| buildExpirationSummary, | ||
| categorizeByExpiration, | ||
| DEFAULT_GRAPHQL_DELAY_MS, | ||
| DEFAULT_MAX_UPDATES_PER_RUN, | ||
| delay, | ||
| processExpiredEntities, | ||
| validateCreationDate, | ||
| }; |
There was a problem hiding this comment.
The new helper module expired_entity_cleanup_helpers.cjs lacks test coverage. This module contains critical logic for expiration categorization, entity processing, and summary generation that was previously duplicated across multiple scripts. Given that the codebase has comprehensive test coverage (e.g., expired_entity_search_helpers.test.cjs, close_expired_discussions.test.cjs, close_expired_pull_requests.test.cjs), and this module contains 250 lines of shared business logic, it should have its own test file to verify:
categorizeByExpirationcorrectly categorizes entities by expiration stateprocessExpiredEntitieshandles the processing loop, rate limiting, and error cases correctlybuildExpirationSummarygenerates the expected summary formatbuildNotExpiredSectionhandles different entity counts and formats correctly- Edge cases like invalid dates, empty arrays, and error handling
Contributor
🔍 PR Triage ResultsCategory: refactor | Risk: medium | Priority: 52/100 Scores Breakdown
📋 Recommended Action: batch_reviewThis is a well-structured refactoring PR that addresses code duplication detected by the duplicate-code-detector workflow. The changes are moderate in scope (net -209 lines) and improve maintainability by extracting shared expiration processing logic. The PR is ready for review (not draft) and has received some review comments. Once CI passes, it should be reviewed alongside other JavaScript refactoring PRs in batch Batch ID: Triaged by PR Triage Agent on 2026-02-03T12:19
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Duplicate expiration evaluation and summary logic in the issue/PR/discussion cleanup scripts made drift likely and maintenance costly. This refactor centralizes the shared flow while preserving per-entity specifics.
expired_entity_cleanup_helpers.cjsto handle expiration parsing, rate-limited processing, and standardized summaries for any entity type.close_expired_issues.cjs,close_expired_pull_requests.cjs, andclose_expired_discussions.cjsnow delegate categorization, processing, and summary rendering to the helper; discussion-specific skip/duplicate handling is preserved via callback.Example (per-entity processor hook):
Original prompt