Skip to content

fix: fix resolution of various type annotation edge cases - #968

Merged
avivkeller merged 3 commits into
nodejs:mainfrom
pimterry:fix-ref-parsing
Aug 3, 2026
Merged

fix: fix resolution of various type annotation edge cases#968
avivkeller merged 3 commits into
nodejs:mainfrom
pimterry:fix-ref-parsing

Conversation

@pimterry

Copy link
Copy Markdown
Member

Description

This fixes a cluster of related issues:

Some changes to other logic (detecting empty brackets and properly handling . separators) required en route, to avoid regressions from the expanded detection from above fixes.

Validation

Rebuilt locally, each of the cases above now render as expected (with one caveat that HTTP/2 Raw Headers isn't linked yet due to nodejs/node#64872). I've skimmed through the rest of the docs and it doesn't look like this affects any other cases.

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run node --run test and all tests passed.
  • I have check code formatting with node --run format:check & node --run lint.
  • I've covered new added functionality with unit tests if necessary.

@pimterry
pimterry requested a review from a team as a code owner July 31, 2026 16:34
Copilot AI review requested due to automatic review settings July 31, 2026 16:34
@vercel

vercel Bot commented Jul 31, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
api-docs-tooling Ready Ready Preview Aug 3, 2026 11:02am

Request Review

@cursor

cursor Bot commented Jul 31, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes core markdown parsing and type-link resolution used across generated API docs; behavior is broad but covered by new unit tests and limited to documentation output, not runtime APIs.

Overview
Fixes inline {…} type annotation handling in the doc metadata pipeline so Node.js API docs link and highlight types correctly.

Parsing (syntax.mjs) rejects brace spans whose first real character cannot start a type (e.g. prose ({), and U+007D (}).), while still accepting annotations after punctuation, leading whitespace, negatives, and escaped []. Link resolution (resolveTypes.mjs) adds a display-name path: whole map keys, then TypeScript identifiers when SWC parses, then |-split unions and trailing [] (e.g. HTTP/2 Headers Object | vm.Module, HTTP/2 Headers Object[]). Partial unions keep resolvable arms; || in values like req.url || '/' is not treated as a union. Sets typescript on nodes so Shiki only uses TS highlighting when appropriate. Dotted Node links (transformers.mjs) use MODULE_QUALIFIED_NAME so strings with dots in prose or generics are not turned into module URLs.

Rendering (hast.mjs) uses Shiki text for non-TypeScript display names so / and digits are not styled as operators. Tests cover remark, resolve, transformers, and hast behavior.

Reviewed by Cursor Bugbot for commit d87c8d9. Bugbot is set up for automated code reviews on this repo. Configure here.

@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.61%. Comparing base (bacccc3) to head (d87c8d9).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #968      +/-   ##
==========================================
+ Coverage   89.12%   89.61%   +0.48%     
==========================================
  Files         196      197       +1     
  Lines       18083    18416     +333     
  Branches     1667     1724      +57     
==========================================
+ Hits        16116    16503     +387     
+ Misses       1960     1906      -54     
  Partials        7        7              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread packages/core/src/utils/type-annotations/__tests__/hast.test.mjs Fixed

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.

Pull request overview

This PR improves Doc-Kit’s {...} type-annotation pipeline by avoiding false-positive captures in prose, improving reference resolution for “display-name” types (with spaces/slashes) including unions/arrays, and preventing non-TypeScript display names from being syntax-highlighted as TypeScript in the web output.

Changes:

  • Tighten micromark type-annotation recognition so {...} spans whose first non-whitespace character cannot start a type are not tokenized as annotations.
  • Improve type reference resolution to handle |-separated display-name unions and [] arrays, and refine the dotted-name heuristic to true identifier paths only.
  • In the highlighted hast handler, choose typescript vs text highlighting per annotation to avoid incorrect operator/number coloring for display names; add/extend unit tests.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/core/src/utils/type-annotations/syntax.mjs Refines {...} tokenization to avoid capturing prose and to validate plausible type starts.
packages/core/src/utils/type-annotations/hast.mjs Switches highlighting language based on whether the value parsed as TypeScript.
packages/core/src/utils/type-annotations/tests/remark.test.mjs Adds coverage for new prose-rejection and punctuation/whitespace edge cases.
packages/core/src/utils/type-annotations/tests/hast.test.mjs Adds coverage ensuring display names are highlighted as plain text (while still linking).
packages/core/src/generators/metadata/utils/transformers.mjs Tightens dotted-name linking heuristic to identifier-path-like names only.
packages/core/src/generators/metadata/utils/resolveTypes.mjs Adds display-name union/array link resolution and marks TS-parseability for the highlighter.
packages/core/src/generators/metadata/utils/tests/transformers.test.mjs Adds test for the refined dotted-name heuristic behavior.
packages/core/src/generators/metadata/utils/tests/resolveTypes.test.mjs Adds new tests for display-name unions/arrays, partial resolution, warnings, and TS-marking.
.changeset/display-name-type-unions.md Documents the patch-level behavior change in a changeset.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/core/src/utils/type-annotations/__tests__/hast.test.mjs Outdated

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 714e6c8. Configure here.

Comment thread packages/core/src/generators/metadata/utils/resolveTypes.mjs
Comment thread packages/core/src/generators/metadata/utils/transformers.mjs Outdated

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.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (1)

packages/core/src/utils/type-annotations/hast.mjs:70

  • The docstring says highlighting falls back when the type “failed to parse”, but the handler now still highlights unparseable values as plain text when links are resolved via display-name splitting (only node.data.parseError triggers fallback). Updating the wording would keep the documentation accurate.
 * Falls back to the minimal handler when the type failed to parse or nothing
 * resolved (no point paying for highlighting then).

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.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Signed-off-by: Tim Perry <pimterry@gmail.com>
This now correctly warns on || in annotations, and updates tests to
avoid hardcoding colours, and avoid looking like we're trying to do XSS
filtering which seems to have confused the bot checks.
Signed-off-by: Tim Perry <pimterry@gmail.com>
@pimterry

pimterry commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

Fixed your comment and force-pushed to rebase & resolve the conflicts @avivkeller

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.

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.

Comment thread packages/core/src/utils/type-annotations/syntax.mjs
@avivkeller
avivkeller enabled auto-merge (squash) August 3, 2026 15:02
@avivkeller
avivkeller merged commit d3306a2 into nodejs:main Aug 3, 2026
24 checks passed
@github-actions github-actions Bot mentioned this pull request Aug 3, 2026
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.

4 participants