Extract shared helper for close-older entity flows#15933
Conversation
- Created close_older_entities.cjs with shared control flow - Refactored close_older_issues.cjs to use shared helper - Refactored close_older_discussions.cjs to use shared helper - Added comprehensive tests for shared helper - All existing tests still passing (25 tests) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Extracts the duplicated “search → limit → iterate → comment → close → log/errors” flow from closeOlderIssues and closeOlderDiscussions into a shared helper, keeping entity-specific behavior behind a configuration object.
Changes:
- Added
closeOlderEntities()helper to centralize close-older control flow with callback-based customization. - Refactored issue + discussion flows to call the shared helper (reducing duplicate logic and consolidating logging/error handling).
- Added a new unit test suite for the shared helper.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| actions/setup/js/close_older_entities.cjs | New shared orchestration helper for closing older entities via config callbacks. |
| actions/setup/js/close_older_entities.test.cjs | New Vitest suite validating the shared helper behavior (limits, errors, args, etc.). |
| actions/setup/js/close_older_issues.cjs | Refactors issues flow to use shared helper and maps helper result back to issue return type. |
| actions/setup/js/close_older_discussions.cjs | Refactors discussions flow to use shared helper; adjusts GraphQL helpers to match shared callback signature. |
Comments suppressed due to low confidence (1)
actions/setup/js/close_older_entities.cjs:85
closeOlderEntitieslogsentity.numberandentity.titledirectly, but those fields are not part of theEntityCloseConfigcontract (onlygetEntityId/getEntityUrlare). If you intend this to be reusable, add config callbacks for display label/number/title (or aformatEntityForLoghook) so the helper doesn’t silently depend on a specific object shape.
core.info(`Found ${olderEntities.length} older ${config.entityType}(s) matching the criteria`);
for (const entity of olderEntities) {
core.info(` - ${config.entityType.charAt(0).toUpperCase() + config.entityType.slice(1)} #${entity.number}: ${entity.title}`);
if (entity.labels) {
core.info(` Labels: ${entity.labels.map(l => l.name).join(", ") || "(none)"}`);
}
core.info(` URL: ${config.getEntityUrl(entity)}`);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -5,26 +5,18 @@ const { getCloseOlderDiscussionMessage } = require("./messages_close_discussion. | |||
| const { getErrorMessage } = require("./error_helpers.cjs"); | |||
There was a problem hiding this comment.
getErrorMessage is imported but never used in this refactored file (error formatting now happens inside close_older_entities.cjs). Please remove the unused require to avoid dead code and keep lint/ts-check clean.
| const { getErrorMessage } = require("./error_helpers.cjs"); |
| // Map to issue-specific return type | ||
| return result.map(item => ({ | ||
| number: item.number, | ||
| html_url: item.html_url || "", |
There was a problem hiding this comment.
This mapping now falls back to an empty string for html_url, but the function’s return contract is html_url: string and callers typically expect a valid URL. Prefer returning item.html_url directly (and/or asserting it’s present) so missing URLs fail fast instead of silently returning invalid links.
| html_url: item.html_url || "", | |
| html_url: item.html_url, |
| // Map to discussion-specific return type | ||
| return result.map(item => ({ | ||
| number: item.number, | ||
| url: item.url || "", |
There was a problem hiding this comment.
This mapping now falls back to an empty string for url, which can silently produce invalid links while the function’s return type indicates url: string. Prefer returning item.url directly (and/or validating it’s present) so unexpected missing URLs don’t get masked.
| url: item.url || "", | |
| url: item.url, |
| core.info(`Search criteria: workflow-id marker: "${getWorkflowIdMarkerContent(workflowId)}"`); | ||
| core.info(`New ${config.entityType} reference: #${newEntity.number} (${newEntity.url || newEntity.html_url})`); | ||
| core.info(`Workflow: ${workflowName}`); | ||
| core.info(`Run URL: ${runUrl}`); | ||
| core.info(""); | ||
|
|
||
| // Step 1: Search for older entities | ||
| const olderEntities = await config.searchOlderEntities(github, owner, repo, workflowId, ...extraArgs, newEntity.number); | ||
|
|
There was a problem hiding this comment.
The helper claims to be entity-agnostic, but it hard-requires newEntity.number (and passes it to searchOlderEntities) rather than using a config callback. Consider adding something like getNewEntityNumber (or reusing getEntityId semantics) so the helper doesn’t bake in an assumption that every entity has a numeric number field.
This issue also appears on line 78 of the same file.
| const start = Date.now(); | ||
| await delay(50); | ||
| const elapsed = Date.now() - start; | ||
| expect(elapsed).toBeGreaterThanOrEqual(45); // Allow some tolerance |
There was a problem hiding this comment.
This test relies on wall-clock timing (Date.now() + a real setTimeout) which can be flaky under CI load and slows the suite unnecessarily. Prefer using fake timers (e.g., advance timers and assert setTimeout was scheduled) or spying on setTimeout to make the test deterministic.
| const start = Date.now(); | |
| await delay(50); | |
| const elapsed = Date.now() - start; | |
| expect(elapsed).toBeGreaterThanOrEqual(45); // Allow some tolerance | |
| vi.useFakeTimers(); | |
| const promise = delay(50); | |
| await vi.advanceTimersByTimeAsync(50); | |
| await expect(promise).resolves.toBeUndefined(); | |
| vi.useRealTimers(); |
closeOlderIssuesandcloseOlderDiscussionscontained ~140 lines of duplicate control flow (search → limit → iterate → close → error handling). Only entity-specific API calls differed.Changes
New shared helper
close_older_entities.cjsexpired_entity_main_flow.cjs)Refactored callers to configuration-based invocation
close_older_issues.cjs: 308 → 222 lines (-28%)close_older_discussions.cjs: 310 → 240 lines (-23%)Before:
After:
Files
actions/setup/js/close_older_entities.cjs(new, 169 lines)actions/setup/js/close_older_entities.test.cjs(new, 11 tests)actions/setup/js/close_older_issues.cjs(refactored)actions/setup/js/close_older_discussions.cjs(refactored)Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
https://api.github.com/repos/github/gh-aw/contents/.github%2Fworkflows%2Faudit-workflows.md/opt/hostedtoolcache/node/24.13.0/x64/bin/node /opt/hostedtoolcache/node/24.13.0/x64/bin/node --conditions node --conditions development --experimental-import-meta-resolve --require /home/REDACTED/work/gh-aw/gh-aw/actions/setup/js/node_modules/vitest/suppress-warnings.cjs /home/REDACTED/work/gh-aw/gh-aw/actions/setup/js/node_modules/vitest/dist/workers/forks.js(http block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.