[jsweep] Clean issue_title_dedup.cjs - #47915
Conversation
- Replace parameter reassignment (value = Number.parseInt(...)) with
a local const variable for cleaner, more idiomatic code
- Fix imprecise `@type {any}` annotation on bestMatch to use the
correct type `{{ title: string, distance: number } | null}`
- Add comprehensive test file (issue_title_dedup.test.cjs) with 26
test cases covering parseDeduplicateByTitle, normalizeTitleForDedup,
and findDuplicateByTitle (previously had no tests)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47915 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Refines issue-title deduplication internals and adds focused unit coverage.
Changes:
- Avoids parameter reassignment during configuration parsing.
- Replaces an imprecise JSDoc type.
- Adds 26 tests covering parsing, normalization, and matching.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/issue_title_dedup.cjs |
Improves implementation clarity and type precision. |
actions/setup/js/issue_title_dedup.test.cjs |
Adds comprehensive unit coverage for deduplication utilities. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Medium
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — this is a clean, well-executed improvement with no actionable issues.
📋 Highlights
- ✅ Parameter reassignment eliminated:
const numericreplaces mutatingvalue = ..., making data flow explicit and idiomatic. - ✅ Type annotation sharpened:
@type {any}→{ title: string, distance: number } | nulldocuments the invariant precisely. - ✅ 26 tests added: Cover happy path, boundary values (0, 100, 101, -1), boolean-like strings, floats, and degenerate inputs — solid specification-by-test.
- ✅ Test names read as plain-English specs: Structure is clear and consistent across all three
describeblocks.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 18.3 AIC · ⌖ 7.37 AIC · ⊞ 6.7K
Comment /matt to run again
There was a problem hiding this comment.
Clean, straightforward refactor. The parseDeduplicateByTitle change correctly avoids parameter reassignment with a const numeric local, and the JSDoc fix on bestMatch improves type precision. The new test file follows the established pattern (import + createRequire) and covers the changed logic well. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 14.4 AIC · ⌖ 4.48 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report
📊 Metrics (26 tests)
|
There was a problem hiding this comment.
Clean refactor, solid test coverage
The const numeric replacement for parameter reassignment in parseDeduplicateByTitle is correct and functionally equivalent. The @type annotation fix is accurate. The new test file follows established repo patterns (import.meta.url in .cjs with vitest) and provides thorough coverage including null/undefined, boundary values, floats, fuzzy-match edge cases, and the normalizedTitle pre-computed path. No actionable issues.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 19 AIC · ⌖ 4.5 AIC · ⊞ 5.7K
Comment /review to run again
|
Excellent work on cleaning up ✅ What looks great:
This change is focused, well-tested, and ready for review. 🚀 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
|
|
🎉 This pull request is included in a new release. Release: |
Summary
Cleans up
actions/setup/js/issue_title_dedup.cjsand adds comprehensive test coverage.Context Type
Node.js (pure utility module)
Changes
issue_title_dedup.cjsparseDeduplicateByTitlefunction previously reassigned thevalueparameter (value = Number.parseInt(value, 10)). This is replaced with aconst numericlocal variable for cleaner, more idiomatic JavaScript.@type {any}annotation: ThebestMatchvariable infindDuplicateByTitlenow uses the precise type{ title: string, distance: number } | nullinstead ofany, improving type safety.issue_title_dedup.test.cjs(new file)Added 26 test cases across three
describeblocks:parseDeduplicateByTitle: 12 tests coveringundefined/null/false/true, boolean-like strings ("true","false"), integer values (0, 5, 10, 100), out-of-range values (101, -1), float, and invalid strings.normalizeTitleForDedup: 5 tests covering lowercasing, space collapsing, trimming, empty string, and mixed cases.findDuplicateByTitle: 7 tests covering empty candidates, exact match at distance 0, no match within threshold, closest-match selection,normalizedTitleusage, best-match ordering, and match exceedingmaxDistance.Validation
✅ Formatting:
npm run format:cjs✓✅ Linting:
npm run lint:cjs✓✅ Type checking:
npm run typecheck✓✅ Tests:
npm run test:js— 26 new tests all pass ✓