feat: semantic tokens - #580
Conversation
🦋 Changeset detectedLatest commit: 1a4c5ba The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (16)
📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (11)
WalkthroughAdds full and range semantic-token support to the language server. The change defines token legends and plugin contracts, extracts tokens from Marko tags and TypeScript-generated code, merges and encodes plugin results, advertises LSP capabilities, handles cancellation and document changes, and refreshes tokens after updates. It also adds browser exports, semantic-token fixtures, snapshot generation, unit tests, style/core-tag refactors, and related repository feedback notes. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/language-server/src/service/create-service.ts (1)
188-193: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a consistent failure log channel for rejected plugins.
Every other aggregator in this file silently drops rejected plugin results; this is the only one that writes to
console.error. If the intent is to surface plugin failures, prefer routing through the server connection logger so stdio transports and client logs stay consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/language-server/src/service/create-service.ts` around lines 188 - 193, Update the rejected-result handling in the results aggregation loop to use the server connection logger instead of console.error. Preserve the existing continue behavior and route result.reason through the logger already used by the surrounding service, ensuring plugin failure output is consistent across transports.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agent-feedback/bugs.md`:
- Around line 27-31: Clarify the Extracted.sourceRangesAt recommendation in
agent-feedback/bugs.md lines 27-31 and agent-feedback/unclear.md lines 5-9:
document that sourceRangesAt may return multiple ranges and linkedSources
aliases, then state that each consumer must apply validation appropriate to its
required precision rather than universally requiring one exact-width,
byte-identical result. Preserve the warnings about anchor-token expansion and
cross-token spans.
In `@agent-feedback/unclear.md`:
- Around line 11-15: The merge behavior in createService must be documented per
operation rather than justified by a broad “duplicates are harmless” rule. Add
concise comments to the findReferences, doRename, findDocumentHighlights,
findDocumentColors, findDocumentLinks, and findDocumentSymbols merge arms
explaining why each uses rangesOverlap-based deduplication or concatenation,
including the navigation and outline impact of duplicates.
In `@packages/language-server/src/service/types.ts`:
- Around line 81-84: Split the semantic-token contracts around
getSemanticTokens: keep the exported service handler returning SemanticToken[] |
SemanticTokens, but introduce and use a plugin/leaf-provider handler type that
returns only SemanticToken[]. Ensure createService receives the leaf type or
normalizes its result before the Array.isArray(result.value) check so encoded {
data } responses are not silently treated as provider results.
---
Nitpick comments:
In `@packages/language-server/src/service/create-service.ts`:
- Around line 188-193: Update the rejected-result handling in the results
aggregation loop to use the server connection logger instead of console.error.
Preserve the existing continue behavior and route result.reason through the
logger already used by the surrounding service, ensuring plugin failure output
is consistent across transports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a80357e-c08c-48f7-9ee2-31ffe97cb1de
⛔ Files ignored due to path filters (36)
packages/language-server/src/__tests__/fixtures/semantic-tokens/concise/__snapshots__/concise.expected/index.htmlis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/concise/__snapshots__/concise.expected/index.mdis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/concise/__snapshots__/concise.expected/index.tokens.mdis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/concise/__snapshots__/concise.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/core-tags/__snapshots__/core-tags.expected/index.htmlis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/core-tags/__snapshots__/core-tags.expected/index.mdis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/core-tags/__snapshots__/core-tags.expected/index.tokens.mdis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/core-tags/__snapshots__/core-tags.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/custom-tag/__snapshots__/custom-tag.expected/components/my-button.htmlis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/custom-tag/__snapshots__/custom-tag.expected/components/my-button.mdis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/custom-tag/__snapshots__/custom-tag.expected/components/my-button.tokens.mdis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/custom-tag/__snapshots__/custom-tag.expected/components/my-button.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/custom-tag/__snapshots__/custom-tag.expected/index.htmlis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/custom-tag/__snapshots__/custom-tag.expected/index.mdis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/custom-tag/__snapshots__/custom-tag.expected/index.tokens.mdis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/custom-tag/__snapshots__/custom-tag.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/edge-tags/__snapshots__/edge-tags.expected/index.htmlis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/edge-tags/__snapshots__/edge-tags.expected/index.mdis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/edge-tags/__snapshots__/edge-tags.expected/index.tokens.mdis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/edge-tags/__snapshots__/edge-tags.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/expressions/__snapshots__/expressions.expected/index.htmlis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/expressions/__snapshots__/expressions.expected/index.mdis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/expressions/__snapshots__/expressions.expected/index.tokens.mdis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/expressions/__snapshots__/expressions.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/script-blocks/__snapshots__/script-blocks.expected/index.htmlis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/script-blocks/__snapshots__/script-blocks.expected/index.mdis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/script-blocks/__snapshots__/script-blocks.expected/index.tokens.mdis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/script-blocks/__snapshots__/script-blocks.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/style-block/__snapshots__/style-block.expected/index.htmlis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/style-block/__snapshots__/style-block.expected/index.mdis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/style-block/__snapshots__/style-block.expected/index.tokens.mdis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/style-block/__snapshots__/style-block.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/tag-params/__snapshots__/tag-params.expected/index.htmlis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/tag-params/__snapshots__/tag-params.expected/index.mdis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/tag-params/__snapshots__/tag-params.expected/index.tokens.mdis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/semantic-tokens/tag-params/__snapshots__/tag-params.expected/index.tsis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (30)
.changeset/semantic-tokens.mdagent-feedback/bugs.mdagent-feedback/cleanup.mdagent-feedback/perf.mdagent-feedback/unclear.mdpackages/language-server/src/__tests__/fixtures/semantic-tokens/concise/index.markopackages/language-server/src/__tests__/fixtures/semantic-tokens/core-tags/index.markopackages/language-server/src/__tests__/fixtures/semantic-tokens/custom-tag/components/my-button.markopackages/language-server/src/__tests__/fixtures/semantic-tokens/custom-tag/index.markopackages/language-server/src/__tests__/fixtures/semantic-tokens/edge-tags/index.markopackages/language-server/src/__tests__/fixtures/semantic-tokens/expressions/index.markopackages/language-server/src/__tests__/fixtures/semantic-tokens/script-blocks/index.markopackages/language-server/src/__tests__/fixtures/semantic-tokens/script-blocks/util.tspackages/language-server/src/__tests__/fixtures/semantic-tokens/style-block/index.markopackages/language-server/src/__tests__/fixtures/semantic-tokens/tag-params/index.markopackages/language-server/src/__tests__/index.test.tspackages/language-server/src/__tests__/semantic-tokens.test.tspackages/language-server/src/__tests__/util/semantic-tokens.tspackages/language-server/src/browser.tspackages/language-server/src/index.tspackages/language-server/src/service/create-service.tspackages/language-server/src/service/marko/index.tspackages/language-server/src/service/marko/semantic-tokens.tspackages/language-server/src/service/marko/util/get-tag-name-completion.tspackages/language-server/src/service/marko/util/is-core-tag.tspackages/language-server/src/service/script/index.tspackages/language-server/src/service/semantic-tokens.tspackages/language-server/src/service/style/index.tspackages/language-server/src/service/types.tspackages/language-server/src/utils/file.ts
|
Thanks for the contribution, @briancarbone — this is a substantial PR and the core design choices are well reasoned. Plugin ordering making taglib classification win overlaps deterministically, the strict single-mapping/exact-width/identical-text filter on TS classifications, and the standalone The review below was generated by Claude Code, not written by me personally. It checked the branch out locally and ran Confirmed defects
Worth a decision rather than a fix
Minor: Items 1 and 2 are the ones I'd want addressed before merge; 3 and 4 are cheap cleanups in the same pass. |
|
Follow-up on item 1, also generated by Claude Code. Digging into the Every tag variable is emitted as a Cross-checking the emitted tokens against
So the name allowlist is wrong in both directions:
Keying off the This also ties back to item 3: |
|
Hi, @DylanPiercey! Thanks for the feedback; walked through it with Fable and ran a battery of adversarial reviews to tighten it up. Following this sentence, Claude: 1 + follow-up. Went with your option D: resolve the token's symbol to its 2. Fixed in both the plugin and the extractor, where the widening exposed the real bug: the compilers pair this regex with 3. Deleted; reproduced your zero-firings result. The hoist alias's 4. Real legend entry now; verified the test can fail. Plain-script branch: cache, 256KB bound, cancellation, plus tests for all three. Decisions: |
|
Non agent speaking (lol): packages/language-server/src/tests/fixtures/script/class-api-basic/snapshots/class-api-basic.expected/index.md This has been updated in a bad way, it means that a |
15978df to
1a4c5ba
Compare
|
You're absolutely right! I mean: I'll hold my agent accountable. What part of "make no mistake" did it fail to understand... |
|
FYI I appreciate the PR even if just throwing compute/agents at it. But I do generally try to fully review and understand PR's to the Marko ecosystem. Also semantic tokens would be good to have for sure. Anyways I may sick my agent back on it for another pass but just to let you know we're currently limited by my meat brain capacity. |
|
I figured you'd prefer to have Claude read through this PR. Maybe have it take a stab and then compare its plan to this PR. This is to improve the experience where regex is less knowledgeable than an AST and it does make a difference for all editors, although I use Vim personally. |
|
@briancarbone for sure. Just curious are you leveraging https://github.com/marko-js/tree-sitter with vim? |
|
No, that's for Neovim, I suppose. I use Vim 9; I've just submitted preliminary syntax highlighting today using that and this as a resource (and passing ~184 tests as prior art). There are some adjustments required still, though. vim/vim@95da62a |
|
By the way, if you do toss this PR, the agent-feedback content may be useful to you... Fable surfaced a handful of out-of-scope issues during the planning/research phase. |
|
Yes, agent-feedback has been tremendously useful! Hasn't been a ton of good work on the language-server since I instituted it but its been very useful on the actual marko project. |
2 participants
Implements
textDocument/semanticTokens(full and range) for Marko documents, and for plain.ts/.jsfiles so embedders without a native TypeScript service get coverage too. Three commits: a standalone fix forprocessDocresult caching (it never hit, and grew without bound), the feature, and review findings filed inagent-feedback/.Script regions pass TypeScript's classifier2020 output straight through: the legend mirrors its type indices and modifier bits, and a span only becomes a token if it maps back to exactly one source range with byte-identical text, since the extractor copies source into several generated contexts and anything less strict colors the wrong characters. Marko tags classify from the taglib: user tags as
class, core control-flow tags askeyword, other core tags asmacro, while native HTML tags stay with the TextMate grammar. Two corrections keep tokens truthful:readonlyis stripped from<let>variables (they compile to a generatedconst), and a mutated tag variable keeps its variable classification over the generated property access.workspace/semanticTokens/refresh(debounced 1s, capability-gated) when watched-file or config changes invalidate classifications; cancellation returnsRequestCancelledand a request that raced an edit returnsContentModified.projectVersion(repeat requests ~1.4ms; cold classification is ~380ns per generated character), and documents whose generated output exceeds 256KB fall back to grammar highlighting.semanticTokenScopescontribution, matching Volar, Svelte, and Astro.The review also surfaced pre-existing issues, filed in
agent-feedback/with repro steps: stale parses for untitled documents, stale diagnostics after watched-file changes, the parser swallowing the rest of a file after a close tag like</if >, and a missing cancellation token in the TS host.Introduced & refined using Fable 5 and Opus 5.