Improve TypeScript quality skill - #602
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
🟡 Changes recommended
Several rules incorrectly classify valid code or omit required repository-specific guidance.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Improves the TypeScript quality skill with expanded readability, maintainability, test-quality, and language guidance, while updating dependencies.
Changes:
- Adds broader TypeScript and JavaScript review criteria.
- Updates templates, reviewer agents, and shared instructions.
- Refreshes npm dependencies.
File summaries
| File | Summary |
|---|---|
package.json |
Updates dependency versions. |
CLAUDE.md |
Synchronizes skill terminology. |
.github/copilot-instructions.md |
Expands shared coding guidance. |
.claude/skills/typescript-code-and-test-standards/SKILL.md |
Adds review and maintainability rules. |
.claude/skills/typescript-code-and-test-standards/references/test-standards.md |
Adds test-quality criteria. |
.claude/skills/typescript-code-and-test-standards/references/google-typescript-style-digest.md |
Adds language-defect guidance. |
.claude/skills/typescript-code-and-test-standards/assets/project-rules.template.md |
Updates project-rule guidance. |
.claude/skills/typescript-code-and-test-standards/assets/copilot-instructions.template.md |
Updates Copilot guidance. |
.claude/skills/typescript-code-and-test-standards/agents/test-quality-reviewer.md |
Expands test review criteria. |
.claude/skills/typescript-code-and-test-standards/agents/comment-and-jsdoc-auditor.md |
Clarifies linter responsibilities. |
Review details
Suppressed comments (7)
.claude/skills/typescript-code-and-test-standards/SKILL.md:162
- This new bullet says there are “six counts above”, but the section now defines seven counts, including the function-body shape immediately above. That makes the explanation internally inconsistent and can cause reviewers to omit one of the counts. Change it to “the other six counts above” (or otherwise name the six preceding counts).
- **The shape of each function body**, as three numbers taken together: lines in the body, deepest nesting level, and the widest single expression a reader must hold at once. The six counts above measure everything around a function, the file it sits in, the type it takes, the directory it lives in, the signature it presents, and none of them reaches inside one, so a three-hundred-line body nested seven deep passes every one of them. It sits here rather than with the line-by-line reading because a long body is the rare defect that is visible on every line and walked past on all of them: no single line says the body is long, which is the same failure the other six counts exist for.
.claude/skills/typescript-code-and-test-standards/SKILL.md:83
- This review checklist treats every spread of a primitive as a language defect, but object spread of primitives,
null, andundefinedis defined JavaScript behaviour ({...null}produces an empty object). That will make the skill report valid code; limit this item to array-spreading non-iterables, or otherwise distinguish array and object spread as the digest does.
- A language-level defect: `for...in` over an array, a spread of a primitive, `parseInt` without a radix, `NaN` compared with `===`, a `switch` case falling through, a floating promise, an `async` callback handed to `forEach`. Same section.
.claude/skills/typescript-code-and-test-standards/SKILL.md:87
- The PR description explicitly calls out detecting
node:imports, but this new checklist only says to notice a construct that differs from surrounding code. It never tells the reviewer to compare thenode:protocol against bare Node specifiers, so a skill consumer can miss the repository convention documented in.github/copilot-instructions.md:65. Please make that check explicit.
- A construct spelled differently from how the rest of the codebase spells the same thing: a built-in module specifier, `import type` against a value import, an alias against a relative path.
.claude/skills/typescript-code-and-test-standards/references/google-typescript-style-digest.md:78
eval,with, anddebuggerare placed under “Defects” even though the rule is scoped to “production” and depends on project context and intent. That contradicts the next paragraph's claim that this half is provable from the language or runtime alone, and it will make the skill report valid context-dependent code as a defect. Keep only the language/runtime defect here, and move the policy-dependent prohibitions to Preferences with the existing project-convention caveat.
- No `eval`, `with`, `debugger` in production, builtin prototype modification, or the `Array()` and `Object()` constructors. `Array(3)` builds three empty slots rather than an element.
.claude/skills/typescript-code-and-test-standards/references/google-typescript-style-digest.md:80
- 🟡 The unqualified
sort()rule is also listed under language defects, but default lexicographic sorting is correct for strings and other intentionally ordered values; only numeric sorting requires a comparator. A reviewer cannot determine that the input is numeric from this rule alone, so qualify it as a numeric-sorting case or move it to preferences to avoid false positives.
- `sort()` compares by string by default, so sorting numbers without a comparator puts 10 before 9.
.claude/skills/typescript-code-and-test-standards/references/google-typescript-style-digest.md:72
- 🔴 This entry is classified as a language defect, but
varis not intrinsically incorrect: the described closure problem occurs only when a loop binding is captured, while manyvardeclarations do not create that situation. Because the skill's own boundary requires defects to be provable without knowing program intent, move this rule to the preference section or qualify it around the closure case rather than flagging everyvardeclaration.
- Never `var`. Its function scoping makes a binding captured inside a loop hold the loop's final value in every closure.
.github/copilot-instructions.md:149
- This shared review guidance repeats two false classifications in the always-report defect list. Object spread of primitives,
null, andundefinedis defined JavaScript behaviour, and arrow-function class properties are valid JavaScript whosethissemantics depend on intent; both are correctly listed as preferences in the digest. Keeping them here will make Copilot flag valid code without a language-level defect.
- **Language defects**, each provable from the language rather than from intent: never spread a primitive, `null`, or `undefined`; give `parseInt` a radix; check `NaN` explicitly, since it compares unequal to itself; never `Array()` as a constructor, since `Array(3)` builds empty slots; no arrow-function class properties, which obscure `this`; `sort()` compares by string, so numbers need a comparator
- Files reviewed: 10/11 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Improve the TypeScript quality skill so it correct identifies and flags the following issues:
node:when not standard in codebaseUpdate npm packages while at it.