Refactor duplicate close-older search flow into a shared helper - #47870
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Triage | Category: Recommended action: Score breakdown: impact=12 (DRY refactor, JS helpers), urgency=10 (draft, no CI), quality=11 (has test file)
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Consolidates duplicate older-entity search logic while preserving entity-specific filtering and mapping.
Changes:
- Adds a shared marker-based search pipeline.
- Migrates issue, pull request, and discussion adapters.
- Adds focused shared-helper tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/close_older_search_helpers.cjs |
Implements the shared search pipeline. |
actions/setup/js/close_older_search_helpers.test.cjs |
Tests shared search behavior. |
actions/setup/js/close_older_issues.cjs |
Adapts issue searches to the helper. |
actions/setup/js/close_older_pull_requests.cjs |
Adapts pull request searches. |
actions/setup/js/close_older_discussions.cjs |
Adapts discussion searches. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Medium
There was a problem hiding this comment.
Clean refactor with no blocking issues. The shared pipeline correctly centralizes marker-query construction, empty-result handling, filtering, logging, and mapping. Entity-specific callbacks preserve all existing behavior. Test coverage is adequate.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 12.9 AIC · ⌖ 4.46 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report
📊 Metrics (26 tests)
✅ Test BreakdownbuildMarkerSearchQuery (8 tests) — Query construction contracts:
filterByMarker (8 tests) — Filtering and exclusion contracts:
Parity: issues and discussions (4 tests) — Cross-entity behavioral equivalence:
searchOlderEntitiesByMarker (3 tests) — Shared pipeline integration:
logFilterSummary (3 tests) — Logging side effects:
Verdict
Strengths:
Note: logFilterSummary tests verify mock calls (logging side effects), which is appropriate for testing side-effect-driven code. All assertions present; no empty test shells.
|
There was a problem hiding this comment.
✅ Clean refactor — approved
This PR correctly extracts the duplicate search pipeline (query → execute → filter → log → map) into a shared searchOlderEntitiesByMarker helper. Behavior is preserved across all three entity types.
Review notes
categoryId closure capture — The categoryId parameter in searchOlderDiscussions is correctly captured by the additionalFilter arrow function closure (line 80 in the new file). No filtering regression.
Pluralization — ${entityType}s correctly produces issues, discussions, pull
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 55.6 AIC · ⌖ 4.5 AIC · ⊞ 5.7K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — no blocking issues; the consolidation is well-executed, but three gaps are worth closing before this ships.
📋 Key Themes & Highlights
Key Themes
- Missing test:
additionalExcludeNumbersforwarding — the only caller-specific path not covered by the newsearchOlderEntitiesByMarkertests. A silent regression here would cause duplicate same-run issues to leak through. - Fragile pluralisation —
${entityType}sworks today for all three callers by coincidence; adding an optionalentityTypePluralparam would make the API explicit and future-proof. - No test for
executeSearchrejection — worth pinning the propagation contract so a future try/catch refactor doesn't accidentally swallow errors.
Positive Highlights
- ✅ The callback-based extraction (executeSearch / getItems / mapItem / additionalFilter) is a clean, flat interface — no deep inheritance or over-abstraction.
- ✅ All entity-specific exclusions correctly remain local to each caller.
- ✅ The three new tests cover the happy path, the early-exit (no workflowId), and the empty-result shape — solid baseline coverage.
- ✅ Net -162 / +93 line delta is a meaningful reduction in duplicated logic.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 52.3 AIC · ⌖ 4.98 AIC · ⊞ 6.7K
Comment /matt to run again
| expect(global.core.info).toHaveBeenCalledWith("No results returned from search API"); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[/tdd] Missing test: additionalExcludeNumbers is not exercised inside searchOlderEntitiesByMarker tests, even though close_older_issues.cjs is the only caller that passes it. A regression that drops the forwarding in the helper would go undetected.
💡 Suggested test
it('should respect additionalExcludeNumbers from the caller', async () => {
const executeSearch = vi.fn().mockResolvedValue({
data: {
items: [
{ number: 1, title: 'Old issue', body: '<!-- gh-aw-workflow-id: wf -->' },
{ number: 5, title: 'Same-run issue', body: '<!-- gh-aw-workflow-id: wf -->' },
],
},
});
const result = await searchOlderEntitiesByMarker({
owner: 'owner',
repo: 'repo',
workflowId: 'wf',
excludeNumber: 99,
entityType: 'issue',
additionalExcludeNumbers: new Set([5]),
executeSearch,
getItems: r => r?.data?.items,
mapItem: item => ({ number: item.number }),
});
expect(result).toEqual([{ number: 1 }]);
});@copilot please address this.
| const filtered = filteredItems.map(mapItem); | ||
|
|
||
| logFilterSummary({ | ||
| entityTypePlural: `${entityType}s`, |
There was a problem hiding this comment.
[/codebase-design] Naive pluralisation (${entityType}s) breaks for "pull request" → logs "pull requests" correctly by coincidence, but would silently produce wrong output for any irregular plural. The existing callers pass entityType: "pull request" (with a space), yielding entityTypePlural: "pull requests" which happens to be correct — but this is fragile.
💡 Suggested fix
Add an optional entityTypePlural param (already accepted by logFilterSummary) and let callers supply it when the default is wrong:
// in searchOlderEntitiesByMarker params
entityTypePlural = `${entityType}s`,Then pass entityTypePlural through to logFilterSummary and the info logs. Callers that already have irregular names (like "pull request") can override it.
@copilot please address this.
| core.info("No results returned from search API"); | ||
| return []; | ||
| } | ||
|
|
There was a problem hiding this comment.
[/tdd] No test covers an executeSearch rejection (network error / API error). The shared helper has no try/catch, so the promise rejection propagates — but a test that asserts this behaviour would prevent future accidental swallowing of errors.
💡 Suggested test
it('should propagate executeSearch rejections', async () => {
const err = new Error('API failure');
await expect(
searchOlderEntitiesByMarker({
owner: 'owner', repo: 'repo', workflowId: 'wf',
excludeNumber: 1, entityType: 'issue',
executeSearch: () => Promise.reject(err),
getItems: r => r?.data?.items,
mapItem: item => item,
})
).rejects.toThrow('API failure');
});@copilot please address this.
|
🎉 This pull request is included in a new release. Release: |
The close-older helpers for issues, pull requests, and discussions each reimplemented the same search pipeline: build a marker query, execute search, filter results, and map matches. This change consolidates that flow so search semantics stay aligned across all three entity types.
Shared search pipeline
searchOlderEntitiesByMarker(...)inactions/setup/js/close_older_search_helpers.cjsEntity-specific adapters kept local
close_older_issues.cjsclose_older_pull_requests.cjsclose_older_discussions.cjsBehavior preserved
Focused test coverage
close_older_search_helpers.test.cjs