[jsweep] Clean update_entity_helpers.cjs#41166
Conversation
- Add specific JSDoc typedefs: EntityUpdateItem, EntityUpdateConfig, EntityUpdateData (with index signature for caller-added fields), and EntityUpdateResult to replace generic Object annotations - Expand test coverage from 6 to 12 tests, adding: - Error thrown when body is present with no resolvable operation - footer:false produces _includeFooter:false - Empty item returns hasCommonUpdates:false with correct _includeFooter - body NOT in updateData.body by default (includeBodyInApiData omitted) - title-only update without body operation - onBodyDisallowed not called when body is absent Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #41166 does not have the 'implementation' label and has 0 new lines of code in business logic directories (well under the 100-line threshold). Files changed: 2. |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
This PR performs a jsweep “unbloat” cleanup of the Node.js helper buildCommonEntityUpdateData() by improving its JSDoc typing (for better editor hover/typechecking ergonomics) and expanding unit test coverage for key guard/default behaviors.
Changes:
- Replaced broad
ObjectJSDoc annotations with explicit@typedefshapes for item/config/return types, and clarified theoptionsparameter type inline. - Added 6 new Vitest cases to cover error-path behavior, no-op defaults, footer propagation, body inclusion defaults, and guard conditions.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/update_entity_helpers.cjs | Introduces JSDoc typedefs (EntityUpdateItem/Config/Data/Result) and updates the function’s JSDoc signatures without changing runtime logic. |
| actions/setup/js/update_entity_helpers.test.cjs | Adds additional tests validating default/guard behavior and error handling for buildCommonEntityUpdateData(). |
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
There was a problem hiding this comment.
JSDoc improvements are positive overall, but three gaps weaken the value of the new types
No logic changes, no correctness bugs — all 12 tests pass. The typedef additions move clearly in the right direction. Three non-blocking issues are worth addressing before this becomes a template for future jsweep passes.
Findings summary
1. 's undermines explicit property annotations (medium) — The index signature makes and the other named fields decorative rather than enforced; TypeScript resolves indexed access through the index signature, not the named property types. Splitting into a base type + extension would make the named properties do real work.
2. 's critical behavioral note was silently dropped (medium) — The old JSDoc said it was Required
[!WARNING]
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
patchdiff.githubusercontent.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:network: allowed: - defaults - "patchdiff.githubusercontent.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 67.7 AIC · ⌖ 9.77 AIC · ⊞ 5.2K
| */ | ||
|
|
||
| /** | ||
| * @typedef {{ _includeFooter: boolean, title?: string, _operation?: string, _rawBody?: string, body?: string, [key: string]: any }} EntityUpdateData |
There was a problem hiding this comment.
[key: string]: any index signature makes the explicit property types in EntityUpdateData mostly decorative — under @ts-check, when an index signature coexists with named properties, type-checking of those named properties is weakened because the index signature dominates, so updateData["_includeFooter"] = "oops" passes without error.
💡 Suggested improvement
If the goal is to allow arbitrary caller-attached extra fields while still getting type-checking benefit from the named properties, consider splitting:
/**
* `@typedef` {{ _includeFooter: boolean, title?: string, _operation?: string, _rawBody?: string, body?: string }} EntityUpdateDataBase
*/
/**
* `@typedef` {EntityUpdateDataBase & { [key: string]: any }} EntityUpdateData
*/This keeps the extensibility while making the named-property annotations do real work. The stated goal of this PR is to replace generic Object with precise types — [key: string]: any on the return type partially defeats that.
| * @param {EntityUpdateConfig} config | ||
| * @param {{ | ||
| * allowTitle?: boolean, | ||
| * defaultOperation?: string, |
There was a problem hiding this comment.
defaultOperation lost its critical behavioral note in the refactoring — the old JSDoc explicitly warned: "Required when item.body may be present; used as fallback operation if item.operation and configDefaultOperation are both absent." The new inline type just shows defaultOperation?: string, silently dropping the constraint that explains the runtime throw in the function body.
💡 Suggested fix
Restore the explanation as an inline JSDoc comment:
* `@param` {{
* allowTitle?: boolean,
* /** Required when item.body may be present; fallback when item.operation and configDefaultOperation are both absent. */
* defaultOperation?: string,
* configDefaultOperation?: string,
* includeBodyInApiData?: boolean,
* onBodyDisallowed?: (() => void),
* }} [options]Without this note, callers who omit defaultOperation and later pass a body will hit an opaque runtime error with no guidance from hover docs.
| */ | ||
|
|
||
| /** | ||
| * @typedef {{ allow_body?: boolean, footer?: any }} EntityUpdateConfig |
There was a problem hiding this comment.
footer?: any is unnecessarily permissive given parseBoolTemplatable is the only consumer — parseBoolTemplatable accepts boolean | string | null | undefined as meaningful inputs. Typing footer as any disables static checking for a field where the accepted values are well-defined by the templatable pattern this module follows.
💡 Suggested fix
/**
* `@typedef` {{ allow_body?: boolean, footer?: boolean | string }} EntityUpdateConfig
*/This reflects actual runtime behavior: parseBoolTemplatable interprets false/"false" as false, anything else as true, and undefined/null as the default. The any type here is a missed opportunity for the type improvement this PR sets out to achieve.
Test Quality Sentinel Report test
|
There was a problem hiding this comment.
������ Test Quality Sentinel Report
✅ Test Quality Score: 90/100 — Excellent
Analyzed 6 test(s): 6 design, 0 implementation, 0 guideline violation(s).
������ Metrics & Test Classification (6 tests analyzed)
| Metric | Value |
|---|---|
| New/modified tests analyzed | 6 |
| ✅ Design tests (behavioral contracts) | 6 (100%) |
| 0 (0%) | |
| Tests with error/edge cases | 4 (67%) |
| Duplicate test clusters | 0 |
| Test inflation detected | No (ratio 1.65x) |
| ������ Coding-guideline violations | 0 |
| Test | File | Classification | Issues Detected |
|---|---|---|---|
throws when body is present but no operation is resolvable |
update_entity_helpers.test.cjs:81 |
✅ Design | — |
returns hasCommonUpdates false when neither title nor body is present |
update_entity_helpers.test.cjs:87 |
✅ Design | — |
populates _includeFooter false when config.footer is false |
update_entity_helpers.test.cjs:96 |
✅ Design | — |
does not include body in updateData.body by default when includeBodyInApiData is omitted |
update_entity_helpers.test.cjs:101 |
✅ Design | — |
handles title-only update without body operation |
update_entity_helpers.test.cjs:108 |
✅ Design | — |
does not call onBodyDisallowed when body is absent even if allow_body is false |
update_entity_helpers.test.cjs:116 |
✅ Design | — |
Go: 0; JavaScript: 6 (*.test.cjs). No other languages detected.
Test inflation: production +26 lines, tests +43 lines — ratio 1.65x (threshold 2x, no penalty).
Verdict
✅ Check passed. 0% implementation tests (threshold: 30%). All 6 new tests verify observable behavior: error-throwing guard, empty-input edge cases, default-option boundary, title-only path, footer config flag, and negative-callback guard.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /improve-codebase-architecture and /tdd — approving with minor suggestions.
📋 Key Themes & Highlights
Minor Improvements
footer?: anyinEntityUpdateConfigcould be narrowed toboolean | string | null— the typedef's goal is precision, soanyhere partially undermines that.operation?: stringinEntityUpdateItemcould be a union type — the domain schema already defines"replace" | "append" | "prepend".- Footer string test — the new
config.footer: falsetest covers the boolean case but misses the"false"string case thatparseBoolTemplatablespecifically handles for resolved GH Actions expressions.
Positive Highlights
- ✅ Clean separation of the four typedef concerns (
Item,Config,Data,Result) - ✅ Doubling test coverage from 6 → 12 with well-named, behaviour-specifying tests
- ✅
[key: string]: anyindex signature onEntityUpdateDatais the right call — accurately models how callers attachstate,labels,assignees, etc. - ✅ PR description is thorough and clearly documents every new test case
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 58.8 AIC · ⌖ 10.9 AIC · ⊞ 6.5K
| */ | ||
|
|
||
| /** | ||
| * @typedef {{ allow_body?: boolean, footer?: any }} EntityUpdateConfig |
There was a problem hiding this comment.
[/improve-codebase-architecture] footer?: any is overly broad for a typedef whose goal is to make types explicit.
Since parseBoolTemplatable is documented to handle boolean | string | null | undefined (literal booleans, string "false", and GH Actions expression strings), the type can be narrowed to express the real contract.
💡 Suggested narrowing
/**
* `@typedef` {{ allow_body?: boolean, footer?: boolean | string | null }} EntityUpdateConfig
*/This admits every value parseBoolTemplatable accepts while eliminating the any escape hatch — making this typedef more useful than the original Object.
| it("populates _includeFooter false when config.footer is false", () => { | ||
| const result = buildCommonEntityUpdateData({}, { footer: false }); | ||
|
|
||
| expect(result.updateData._includeFooter).toBe(false); |
There was a problem hiding this comment.
[/tdd] The new footer test covers footer: false (boolean), but parseBoolTemplatable specifically handles the string "false" as a key case for resolved GitHub Actions expressions.
Adding a parallel test for config.footer = "false" would validate the templatable contract that the typedef's any type is designed to accommodate.
💡 Suggested additional test
it('populates _includeFooter false when config.footer is the string "false"', () => {
const result = buildCommonEntityUpdateData({}, { footer: 'false' });
expect(result.updateData._includeFooter).toBe(false);
});This is the scenario that arises when a GitHub Actions workflow passes footer: ${{ inputs.include-footer }} and the runner resolves it to the string "false".
|
@copilot resolve the 6 open review threads after the branch update, then re-request review.
|
|
``
|
|
``
|
|
@copilot run pr-finisher skill |
|
@copilot review all comments and address unresolved review feedback.
|
|
Please refresh the branch and rerun checks after the review fixes land.
|
…restore defaultOperation note, add footer string test Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 28bf1f5. Applied all review feedback:
|
Summary
Cleans up
update_entity_helpers.cjsas part of thejsweepinitiative. The change adds structured JSDoc type definitions and expands unit-test coverage; no runtime behaviour is altered.Changes
actions/setup/js/update_entity_helpers.cjs@typedefdeclarations —EntityUpdateItem,EntityUpdateConfig,EntityUpdateDataBase,EntityUpdateData, andEntityUpdateResult— giving the module a formal, self-documenting type surface.buildCommonEntityUpdateDataJSDoc comment to reference the new typedefs and restructure theoptionsparameter block for clarity.footerfield type and splitEntityUpdateDatain response to review feedback; restored thedefaultOperationnote that had been dropped.actions/setup/js/update_entity_helpers.test.cjsbuildCommonEntityUpdateData:config.footer(including a new string-valued footer test added post-review).includeBodyInApiDatawhen not applicable.onBodyDisalloweddoes not fire when body is absent.Impact Assessment
Commit Lineage
e22f1371f84c82128328bf1f55e