[jsweep] Clean update_pr_description_helpers.cjs#36575
Conversation
- Deduplicate island content building in replace-island branch - Add 4 new tests: historyUrl, workflowIdMarker with/without workflowId - All 40 tests pass (was 36) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #36575 does not have the 'implementation' label (has_implementation_label=false) and has 0 new lines of code in business logic directories (default_business_additions=0, well below the 100-line threshold). requires_adr_by_default_volume=false. |
There was a problem hiding this comment.
Pull request overview
Refactors update_pr_description_helpers.cjs to reduce duplicated string construction in the replace-island path, and extends the unit tests to cover historyUrl propagation and includeFooter: false workflow-id marker behavior.
Changes:
- Hoist
startMarker,endMarker, andislandContentconstruction inupdateBody()’sreplace-islandbranch to eliminate duplicated code. - Add tests covering
historyUrlbeing included in the generated footer output. - Add tests ensuring
<!-- gh-aw-workflow-id: ... -->is emitted (or omitted) whenincludeFooter: falsedepending on whetherworkflowIdis non-empty.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/update_pr_description_helpers.cjs | Deduplicates island marker/content construction for the replace-island operation. |
| actions/setup/js/update_pr_description_helpers.test.cjs | Adds coverage for historyUrl and includeFooter: false workflow-id marker behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
![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 /zoom-out — approving with one minor observation on test precision.
📋 Key Themes & Highlights
Key Themes
- Clean deduplication: Hoisting the three
startMarker/endMarker/islandContentvariables above theif/elseis semantically equivalent and clearly correct — all variables are derived from stable locals (workflowId,contentWithCaution, etc.) that do not change between the two branches. - Good test additions: The four new tests cover previously untested paths (
historyUrlpropagation,workflowIdmarker presence/absence whenincludeFooter: false), which lock in the behaviour and validate the refactor is safe.
Positive Highlights
- ✅ Refactor is a strict deduplication — no logic change
- ✅ Net -5 lines in the implementation file
- ✅ All CI checks pass (format, lint, typecheck, 40 tests)
- ✅ One inline suggestion about test assertion specificity — non-blocking
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.1M
| }); | ||
| expect(result).toContain("New content"); | ||
| // historyUrl should be included in the footer | ||
| expect(result).toContain("https://github.com/search?q=test"); |
There was a problem hiding this comment.
[/zoom-out] The historyUrl test verifies that the URL appears somewhere in the output, but does not assert it lands specifically inside the generated footer. If historyUrl were accidentally placed in the main content, the test would still pass.
💡 Suggestion
Consider asserting on the specific footer fragment that carries the URL:
expect(result).toContain(`[◷](https://github.com/search?q=test)`);
// or
expect(result).toMatch(/Generated by.*https:\/\/github\.com\/search/);This guards against regressions where the URL is injected in the wrong location.
There was a problem hiding this comment.
✅ Clean deduplication — no blocking issues
The refactor correctly hoists the shared islandContent construction before the if (island.found) branch. Both the found and fallback paths used identical construction code before, so the hoist is semantically equivalent. New tests cover historyUrl, workflowIdMarker with and without a workflowId, which are the right things to assert on.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 87.1K
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (4 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
Summary
Refactors
updateBodyinupdate_pr_description_helpers.cjsto eliminate duplicated island marker and content construction by hoisting shared variables above theif/elsebranches, and extends the test suite with coverage for the optionalhistoryUrlparameter andworkflowIdMarkerinclusion/exclusion whenincludeFooteris false.Changes
actions/setup/js/update_pr_description_helpers.cjs(modified)if/elsebranches inupdateBody, removing duplication between both code paths.actions/setup/js/update_pr_description_helpers.test.cjs(modified)historyUrlparameter.workflowIdMarkeris excluded whenincludeFooterisfalse.Risk Assessment