Skip to content

Improve TypeScript quality skill - #602

Merged
AlexJSully merged 3 commits into
mainfrom
alexjsully-260917-fix-ts-skill
Sep 17, 2026
Merged

AlexJSully merged 3 commits into
mainfrom
alexjsully-260917-fix-ts-skill

Conversation

@AlexJSully

Copy link
Copy Markdown
Owner

Improve the TypeScript quality skill so it correct identifies and flags the following issues:

  • Usage of node protocol imports via node: when not standard in codebase
  • Flag bloat or useless tests
  • Single like if return that are not human readable
  • Overall code maintainability and human readability

Update npm packages while at it.

@AlexJSully
AlexJSully requested a lite review from Copilot September 17, 2026 19:10
@AlexJSully AlexJSully self-assigned this Sep 17, 2026
@vercel

vercel Bot commented Sep 17, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
portfolio Ready Ready Preview Sep 17, 2026 8:02pm UTC

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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, and undefined is 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 the node: 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, and debugger are 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 var is not intrinsically incorrect: the described closure problem occurs only when a loop binding is captured, while many var declarations 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 every var declaration.
- 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, and undefined is defined JavaScript behaviour, and arrow-function class properties are valid JavaScript whose this semantics 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.

@AlexJSully
AlexJSully merged commit 8515e73 into main Sep 17, 2026
2 checks passed
@AlexJSully
AlexJSully deleted the alexjsully-260917-fix-ts-skill branch September 17, 2026 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants