Uh oh!
There was an error while loading. Please reload this page.
[jsweep] Clean upload_assets.cjs - #48280
Conversation
Import normalizeBranchName from normalize_branch_name.cjs instead of maintaining a copy. The two implementations were identical (upload_assets lacked the optional salt parameter which defaults to null and has no effect when omitted). Also remove the stale sync-reminder comment from normalize_branch_name.cjs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #48280 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. No test files were added or modified in this PR. This is a code cleanup PR ([jsweep] Clean upload_assets.cjs) affecting only production files (actions/setup/js/normalize_branch_name.cjs, actions/setup/js/upload_assets.cjs). Test Quality Sentinel analysis not applicable. |
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Centralizes branch-name normalization by reusing the canonical helper.
Changes:
- Imports
normalizeBranchNameinupload_assets.cjs. - Removes duplicate implementation and stale synchronization comment.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/upload_assets.cjs | Replaces duplicated logic with shared helper. |
actions/setup/js/normalize_branch_name.cjs | Removes obsolete synchronization guidance. |
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
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — clean deduplication, no blocking issues.
📋 Highlights
Positive Highlights
- ✅ Eliminates the duplicated
normalizeBranchNameimplementation — single source of truth - ✅ Removes the stale "keep in sync" comment, a maintenance hazard
- ✅ Import correctly delegates to
normalize_branch_name.cjs - ✅ No behaviour change:
saltparam defaults tonullwith no effect when omitted
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 16.2 AIC · ⌖ 4.52 AIC · ⊞ 6.7K
Comment /matt to run again
🧪 Test Quality Sentinel Report✅ Test Quality Score: N/A — No Test Files
Verdict
|
There was a problem hiding this comment.
✅ LGTM — clean deduplication
The duplicate normalizeBranchName implementation has been correctly removed from upload_assets.cjs and replaced with an import from the canonical normalize_branch_name.cjs. The optional salt parameter defaults to null and is not passed at the call site, so runtime behavior is identical. No issues found.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 15.3 AIC · ⌖ 4.5 AIC · ⊞ 5.7K
Comment /review to run again
There was a problem hiding this comment.
Good cleanup — removes duplicated normalizeBranchName from upload_assets.cjs and imports the canonical implementation from normalize_branch_name.cjs. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 12.1 AIC · ⌖ 4.48 AIC · ⊞ 5K
Uh oh!
There was an error while loading. Please reload this page.
🎉 This pull request is included in a new release. Release: |
Summary
Removes the duplicated
normalizeBranchNamefunction fromupload_assets.cjsand imports it fromnormalize_branch_name.cjsinstead. The two implementations were identical (the canonical version has an optionalsaltparameter that defaults tonulland has no effect when omitted by upload_assets).Also removes the stale "keep in sync" comment from
normalize_branch_name.cjssince it is now the single source of truth.File Cleaned
upload_assets.cjs— github-script contextChanges
normalizeBranchNameimplementationrequireimport ofnormalizeBranchNamefrom./normalize_branch_name.cjsnormalize_branch_name.cjsTest Coverage
No new tests needed — existing 14 tests in
upload_assets.test.cjsalready covernormalizeBranchNamebehavior (including the normalization test at line 121) and all pass with the import-based approach.Validation ✅
npm run format:cjs✓npm run lint:cjs✓npm run typecheck✓npm run test:js✓ (all 14 upload_assets tests pass; other pre-existing failures unrelated to this change)