[jsweep] Clean update_project.cjs#39183
Conversation
- Extract duplicated ~120-line field update loop into shared applyFieldUpdates(github, projectId, itemId, fields) helper, eliminating ~240 lines of duplication across draft_issue and content_number branches - Extract IIFE for item lookup into named findExistingItemByContentId(github, projectId, contentId) function comma-chained consts, IIFE) with clean readable code - Expand GraphQL mutation strings from single-line escaped \n sequences into readable multi-line template literals - Replace index-based for loop with for...of .entries() on configuredViews - Export normalizeUpdateProjectOutput, summarizeProjectsV2, and summarizeEmptyProjectsV2List utility functions for direct testing - Add 22 new tests covering normalizeUpdateProjectOutput (camelCase alias normalization), summarizeProjectsV2 (formatting, filtering, limit), and summarizeEmptyProjectsV2List (diagnostics, SSO hint, empty cases) - Tests: 76 → 98 (all passing) 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. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #39183 does not have the 'implementation' label and has 0 new lines of code in business logic directories (≤100 threshold). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the update_project.cjs github-script implementation to reduce duplication and improve readability, while expanding the unit test suite to directly cover newly exported helper utilities.
Changes:
- Extracts shared helpers (
applyFieldUpdates,findExistingItemByContentId) and replaces duplicated inline logic with named functions. - Cleans up previously minified/condensed logic and converts GraphQL strings into readable multiline template literals.
- Exports utility helpers and adds/expands test coverage for output normalization and project-list summarization helpers.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/update_project.cjs | Refactors project-item lookup and field-update logic into helpers; improves readability and exports utilities for testing. |
| actions/setup/js/update_project.test.cjs | Adds new test suites for the newly exported helper utilities and increases overall coverage. |
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
🧪 Test Quality Sentinel Report✅ Test Quality Score: 81/100 — Excellent
📊 Metrics & Test Classification (22 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
|
![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, /tdd, and /improve-codebase-architecture — clean refactor with solid structural improvements; commenting with targeted suggestions (no blockers).
📋 Key Themes & Highlights
Positive Highlights
- ✅ Excellent de-duplication:
applyFieldUpdateseliminates ~240 lines of duplicate field-update logic shared acrossdraft_issueandcontent_numberbranches - ✅
findExistingItemByContentId→ named function replaces the inline IIFE cleanly, with JSDoc and clear pagination logic - ✅ GraphQL template literals are a significant readability win over the minified single-line
\n-escaped strings - ✅ 22 new unit tests with comprehensive coverage of the three exported utilities; Arrange/Assert structure is clear throughout
- ✅
for...of .entries()modernisation is idiomatic and removes the manual index/element split
Suggestions (no blockers)
- Null safety —
result.nodeinfindExistingItemByContentIdcan benullfor invalid project IDs; a guard would convert the opaque TypeError into a clear warning (line 406) - Testability gap —
applyFieldUpdateshas complex field-type inference but no direct unit tests; extracting a pureinferFieldDataTypehelper would unlock exhaustive coverage without mocking (line 424) - Dead condition —
isDateFieldOR-condition includes a redundantincludes("_date")term (line 439) - Missing comment —
"classification"hard-code inisTextFieldlost its explanatory comment in the extraction (line 440) - Fragile test —
toContain("SSO")is editorial-string sensitive; asserting on diagnostic data (totalCount=5,0 readable project nodes) would be more stable (test line 2339)
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 406.8 AIC · ⌖ 40.3 AIC · ⊞ 27.9K
| { projectId, after: endCursor } | ||
| ); | ||
|
|
||
| const found = result.node.items.nodes.find(item => item.content?.id === contentId); |
There was a problem hiding this comment.
[/zoom-out] result.node can be null if projectId is invalid or inaccessible — the GraphQL node() query returns null for unknown/unauthorized IDs. This causes an unguarded TypeError on .items that was previously hidden inside the IIFE; now that it is a named function, making the failure observable is straightforward.
💡 Suggested guard
if (!result?.node?.items) {
core.warning(`Project ${projectId} not found or inaccessible; stopping item search.`);
break;
}
const found = result.node.items.nodes.find(item => item.content?.id === contentId);Without this guard, an invalid projectId surfaces as an opaque TypeError in the updateProject call stack rather than a clear warning.
| * @param {Record<string, unknown>} fields - Field name/value pairs to update | ||
| * @returns {Promise<void>} | ||
| */ | ||
| async function applyFieldUpdates(github, projectId, itemId, fields) { |
There was a problem hiding this comment.
[/tdd] applyFieldUpdates is the highest-risk extracted helper — it contains complex field-type inference heuristics (date/text/single-select detection, type mismatch guards, three field-creation paths) — but has no dedicated unit tests. The PR wisely exports three utility functions for direct testing; the same treatment for this helper would significantly improve regression safety.
💡 Approach
Exporting applyFieldUpdates for testing is tricky because it references the global core and calls fetchAllProjectFields. Consider extracting the pure, side-effect-free detection logic into a small helper:
/** `@returns` {"DATE"|"TEXT"|"SINGLE_SELECT"} */
function inferFieldDataType(fieldName, fieldValue, datePattern) {
const isDateField = fieldName.toLowerCase().includes("date");
const isTextField = fieldName.toLowerCase() === "classification" || (typeof fieldValue === "string" && fieldValue.includes("|"));
if (isDateField && typeof fieldValue === "string" && datePattern.test(fieldValue)) return "DATE";
if (isTextField) return "TEXT";
return "SINGLE_SELECT";
}This pure function can be tested exhaustively without mocking github or core, covering cases like start_date, classification, pipe-delimited values, and the fallback path.
| continue; | ||
| } | ||
|
|
||
| const isDateField = fieldName.toLowerCase().includes("_date") || fieldName.toLowerCase().includes("date"); |
There was a problem hiding this comment.
[/improve-codebase-architecture] The first condition includes("_date") is dead code: every string that contains "_date" also contains "date", so the OR short-circuits to the second term in all cases. The condition simplifies to just includes("date") with no behavioural change.
💡 Simplified
const isDateField = fieldName.toLowerCase().includes("date");This was pre-existing in the original IIFE, but applyFieldUpdates is now the single canonical location for this logic — a good place to tidy it up.
| } | ||
|
|
||
| const isDateField = fieldName.toLowerCase().includes("_date") || fieldName.toLowerCase().includes("date"); | ||
| const isTextField = fieldName.toLowerCase() === "classification" || (typeof fieldValue === "string" && fieldValue.includes("|")); |
There was a problem hiding this comment.
[/improve-codebase-architecture] The hardcoded "classification" field name is opaque — the extracted helper lost the inline comment from the original code explaining why this specific field name always gets TEXT treatment instead of SINGLE_SELECT.
💡 Suggestion
// "classification" is always treated as a free-text field rather than single-select;
// pipe-delimited values ("A|B|C") also signal a free-text field storing multi-option strings.
const isTextField =
fieldName.toLowerCase() === "classification" ||
(typeof fieldValue === "string" && fieldValue.includes("|"));Alternatively, consider making the list of always-text field names configurable (e.g., a TEXT_FIELD_NAMES constant) so the special-casing is visible and extensible rather than buried in a condition.
|
|
||
| it("includes SSO hint in message when totalCount > 0", () => { | ||
| const result = summarizeEmptyProjectsV2List({ totalCount: 5, diagnostics: { rawNodesCount: 0, nullNodesCount: 0, rawEdgesCount: 0, nullEdgeNodesCount: 0 } }); | ||
| expect(result).toContain("SSO"); |
There was a problem hiding this comment.
[/tdd] toContain("SSO") tests for a specific editorial string fragment, making this test fragile to message wording changes that do not change the semantics.
💡 More stable alternative
it("includes SSO hint in message when totalCount > 0", () => {
const result = summarizeEmptyProjectsV2List({ totalCount: 5, ... });
// Test the data communicated, not the exact wording
expect(result).toContain("totalCount=5");
expect(result).toContain("0 readable project nodes");
});Assertions on "totalCount=5" and "0 readable project nodes" are tied to the data in the diagnostic — they remain valid across message rephrasing while still confirming the diagnostic branch was taken.
There was a problem hiding this comment.
REQUEST_CHANGES — 2 correctness bugs + 2 medium concerns in new helpers
The refactoring direction is right: eliminating ~240 lines of duplication and making the logic readable are clear wins, and the 22 new tests are solid. However, the extraction introduced new code that carries forward null-safety gaps from the old minified code, plus surfaced a pre-existing consistency bug that is now more visible in the shared helper.
Blocking issues (must fix before merge)
Non-blocking (should fix)
-
contentDatanull (line 1142): If the issue/PR doesn't exist intargetRepo, theTypeErrorfromnull.idis caught but the error message is opaque. A pre-emptive null check with a clearERR_VALIDATIONmessage would help operators. -
Case-sensitive single-select vs case-insensitive iteration (line 573): Inconsistency that silently drops updates depending on field type. Fix is one-liner: add
.toLowerCase()normalization on both sides.
🔎 Code quality review by PR Code Quality Reviewer · 537.9 AIC · ⌖ 13.8 AIC · ⊞ 17.3K
| { projectId, after: endCursor } | ||
| ); | ||
|
|
||
| const found = result.node.items.nodes.find(item => item.content?.id === contentId); |
There was a problem hiding this comment.
Unguarded null dereference when project node is not a ProjectV2.
If projectId points to a deleted project, a classic Project (v1), or any non-ProjectV2 node, the ... on ProjectV2 inline fragment yields no fields. result.node becomes {} or null, so result.node.items is undefined and the chain throws TypeError: Cannot read properties of undefined (reading 'nodes'). The outer catch in updateProject rescues the exception, but the message is opaque and doesn't help diagnose the real cause.
💡 Suggested fix
const found = result.node?.items?.nodes.find(item => item.content?.id === contentId);
if (found) return found;
const pageInfo = result.node?.items?.pageInfo;
hasNextPage = pageInfo.hasNextPage;
endCursor = pageInfo.endCursor;Or add a single guard at the top of the loop body:
| } | ||
|
|
||
| let valueToSet; | ||
| if (field.dataType === "DATE") { |
There was a problem hiding this comment.
TypeError crash when createProjectV2Field returns a null projectV2Field — aborts all remaining field updates for this item.
The outer catch in updateProject saves the process, but the entire item's field update is aborted — every subsequent field in the loop is silently lost.
💡 Suggested fix
core.warning(`Field "${fieldName}" could not be created or resolved; skipping.`);
continue;
}This converts a process-aborting TypeError into a per-field warning, keeping other field updates alive.
| }`; | ||
| const contentResult = await github.graphql(contentQuery, { owner: contentOwner, repo: targetRepo, number: contentNumber }); | ||
| const contentData = contentType === "Issue" ? contentResult.repository.issue : contentResult.repository.pullRequest; | ||
| const contentId = contentData.id; |
There was a problem hiding this comment.
contentData.id crashes if the issue or PR number does not exist in the target repo.
contentResult.repository.issue (or .pullRequest) returns null when the content number is valid but doesn't exist in targetRepo (deleted issue, wrong repo, cross-repo reference). Line 1142 then reads null.id and throws. The outer catch surfaces an opaque TypeError rather than a user-actionable message like "Issue #N not found in org/repo".
💡 Suggested fix
const contentData = contentType === "Issue"
? contentResult.repository.issue
: contentResult.repository.pullRequest;
throw new Error(
`${ERR_VALIDATION}: ${contentType} #${contentNumber} not found in ${contentOwner}/${targetRepo}.`
);
}This gives operators a clear diagnostic instead of an unhelpful stack trace.
| } | ||
| valueToSet = { iterationId: iteration.id }; | ||
| } else if (field.options) { | ||
| const option = field.options.find(o => o.name === fieldValue); |
There was a problem hiding this comment.
Case-sensitive single-select lookup silently drops updates that the iteration lookup would accept.
Line 565 (ITERATION) normalizes both sides with .toLowerCase() for matching. Line 573 (single-select options) uses strict === with no normalization. A YAML value of in progress will silently skip an option named In Progress and emit a warning, while the same value for an iteration field would succeed. The inconsistency is a silent data-loss trap for configurations that mix field types.
💡 Suggested fix
const option = field.options.find(
o => o.name.toLowerCase() === String(fieldValue).toLowerCase()
);```
This mirrors the iteration lookup and eliminates the case-sensitivity gap.
</details>
Summary
Cleans
update_project.cjs(71.7 KB, github-script context) by eliminating ~240 lines of duplication, removing minified code, and extracting named helper functions.Changes
Extracted
applyFieldUpdateshelperdraft_issueandcontent_numberbranches contained identical ~120-line field update loopsapplyFieldUpdates(github, projectId, itemId, fields)shared helper\nsequences to readable multi-line template literalsExtracted
findExistingItemByContentIdhelperfindExistingItemByContentId(github, projectId, contentId)functionCleaned minified
contentNumberblocknull !== contentNumber,"pull_request" === output.content_type)!0/!1boolean literals withtrue/falseconstdeclarations and IIFE with clean readable codefor...of .entries()loopfor (let i = 0; i < arr.length; i++)→for (const [i, viewConfig] of configuredViews.entries())Exported utility functions for direct testing
normalizeUpdateProjectOutput,summarizeProjectsV2,summarizeEmptyProjectsV2Listadded tomodule.exportsContext
Execution context: github-script (uses global
core,github,context)@ts-nocheck: Not present — file already had@ts-check; no type errorsTest Improvements
New test suites added:
normalizeUpdateProjectOutput(11 tests): camelCase alias normalization, non-object passthrough, snake_case precedencesummarizeProjectsV2(7 tests): empty/null input, single project, closed project, multiple projects, filtering, limitsummarizeEmptyProjectsV2List(4 tests): empty input, totalCount context, diagnostic counts, SSO hintValidation
All checks passed:
npm run format:cjsnpm run lint:cjsnpm run typechecknpm run test:js— 98/98 passedWarning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
traces.example.comSee Network Configuration for more information.