fix: fix resolution of various type annotation edge cases - #968
Conversation
Signed-off-by: Tim Perry <pimterry@gmail.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview Tokenizer ( Resolution ( Link heuristics ( Rendering ( Reviewed by Cursor Bugbot for commit 2f77a39. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #968 +/- ##
==========================================
+ Coverage 86.63% 87.16% +0.53%
==========================================
Files 195 196 +1
Lines 17938 18268 +330
Branches 1632 1689 +57
==========================================
+ Hits 15540 15924 +384
+ Misses 2392 2338 -54
Partials 6 6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
typescriptvstexthighlighting 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.
| // TypeScript colours `/` as an operator and `2` as a numeric literal | ||
| assert.match(asTypeScript, /<span style="color:#B48EAD">2<\/span>/); | ||
| assert.doesNotMatch(asDisplayName, /<span style="color:/); |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ 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.
| // A dotted identifier path, e.g. `vm.Module` or `os.constants.dlopen`. Names | ||
| // that merely contain a dot (prose, `Object.<string, string>`) must not be | ||
| // turned into a module link. | ||
| const MODULE_QUALIFIED_NAME = /^[A-Za-z_$][\w$]*(?:\.[A-Za-z_$][\w$]*)+$/; |
There was a problem hiding this comment.
Can you put this in ../constants.mjs?
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.
There was a problem hiding this comment.
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
linksare resolved via display-name splitting (onlynode.data.parseErrortriggers 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).
Description
This fixes a cluster of related issues:
{inline in normal text could break parsing, see the 3rd bullet under https://beta.docs.nodejs.org/url.html#whatwg-api{HTTP/2 Headers Object | HTTP/2 Raw Headers}. The current parsing didn't handle|at all, it just fell back to TS, which works fine for primitives but doesn't handle these cases and printed warnings instead, leaving the links broken, e.g.headersoption in https://beta.docs.nodejs.org/http2.html#clienthttp2sessionrequestheaders-options.HTTP/2 Headers Object[]due to[]broke links, e.g. https://beta.docs.nodejs.org/http2.html#http2streamsentinfoheaders.HTTP/2 ...showing operator highlighting for the/and numeric highlight for the2which looks weird, e.g. https://beta.docs.nodejs.org/http2.html#event-localsettings. We now parse each reference and only apply syntax highlighting on TS-parseable cases.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 Headersisn'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
node --run testand all tests passed.node --run format:check&node --run lint.