Skip to content

Commit 9950951

Browse files
committed
docs(agent-feedback): file eight language-server findings from a third scan wave
1 parent e904210 commit 9950951

8 files changed

Lines changed: 96 additions & 0 deletions
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
type: bug
3+
impact: med
4+
effort: low
5+
site: packages/language-server/src/service/marko/complete/AttrName.ts › AttrName
6+
---
7+
8+
# Keep the script plugin's object-literal properties out of the attribute-modifier position
9+
10+
`AttrName` already gets the modifier position right on its own: with the caret after the colon it returns exactly `scoped` and `no-update`. But `createService.doComplete` unions every plugin's result, and at that same offset the script plugin maps into the generated attributes object literal and adds the tag's whole property list, so `<div class:|>` returns 360 items and `<input value:|/>` returns 398. The two legal items are also buried: they carry no `sortText`, while the TypeScript items carry `"11"`, which sorts ahead of `no-update` and `scoped` in any client that honours `sortText`. This is the position where the server is most needed, because the modifier vocabulary is small, Marko-specific and not guessable, and it is the one position where an exhaustive property list is certainly wrong. Give the plugin facade a way for a handler to answer exclusively, or have the script plugin return nothing when the source offset falls after a colon inside an `AttrName`.
11+
12+
Check: request completion at the caret in `<div class:|>` and `<input value:|/>` against a project on marko 6.3.44; today the results are 360 and 398 items and should be the two modifiers, ordered ahead of anything else the facade merges in.
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
type: bug
3+
impact: med
4+
effort: low
5+
site: packages/language-server/src/service/html/index.ts › doValidate
6+
---
7+
8+
# Publish the axe rule id as a diagnostic `code`, not inside the `source` string
9+
10+
Accessibility diagnostics are published with the rule id interpolated into `source` as `axe-core(<rule>)` and no `code`, so the one stable identifier a client could filter, suppress or look up is glued into a display string that also has to be parsed to recover it: an `<img>` with no alt arrives as `{"source":"axe-core(image-alt)","severity":3,"message":"Fix any of the following: ..."}`. TypeScript diagnostics from the script plugin carry `code: 2304`, so a client cannot treat the two uniformly, and "disable this rule on this line" cannot be built on top of what the server publishes today. The violation axe returns also carries `helpUrl`, which the `flatMap` over `violations` currently drops along with the rest of the rule metadata, so `source: "axe-core"`, `code: ruleId` and `codeDescription: { href: helpUrl }` are all available at that same line. Marko's own parse diagnostics have the same gap, but the compiler's `Diagnostic` interface (`type`, `label`, `loc`, `fix`) has no id to forward, so giving those a code needs an upstream field in marko-js/marko first.
11+
12+
Check: `didOpen` a template containing `<img src="x.png">` and read the published diagnostic; today it is `{"source":"axe-core(image-alt)"}` with no `code`, and it should be `{"source":"axe-core","code":"image-alt"}` with a `codeDescription.href` pointing at the rule's help page.
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
type: bug
3+
impact: high
4+
effort: med
5+
site: packages/language-server/src/service/marko/complete/index.ts › doComplete
6+
---
7+
8+
# Return tag and attribute completions when the typed prefix is still empty
9+
10+
`doComplete` dispatches on `NodeType[node.type]` and falls back to `|| []`, but at an empty prefix there is no `OpenTagName` or `AttrName` node to dispatch to: `parsed.nodeAt` returns `Program` for a document that is only `<`, and `Tag` for the caret in `<div |>` or `<user-card |/>`. So `textDocument/completion` answers zero items at exactly the positions the server itself nominates as `completionProvider.triggerCharacters` -- `<` and space -- while one more character works: `<le` returns 295 items, `<user-card u` returns the tag's two attributes, `<div class:s` returns the two modifiers. Typing `<` and expecting the tag list is the first gesture a new user makes, and listing a component's attributes without opening its source is the main reason to run the server at all; both silently return nothing at both `triggerKind` 1 and 2. `Tag` already handles a caret inside an unfinished tag for the closing-tag completion, so the dispatch can recognize these positions: when the caret sits immediately after `<`, or in the whitespace of an open tag, run `OpenTagName`/`AttrName` against an empty range at that offset.
11+
12+
Check: drive the built server over stdio, `didOpen` a document whose entire content is `<`, and request completion at 0:1; today the result is `{"items":[],"isIncomplete":true}` and it should be the tag list that `<le` already returns. Same for the caret in `<div |>hi</div>` and `<user-card |/>`, which should return those tags' attributes.
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
type: dx
3+
impact: med
4+
effort: low
5+
site: packages/language-server/src/service/marko/document-symbols.ts › extractDocumentSymbols
6+
---
7+
8+
# Name a Marko variable tag after its variable, and return a nested `DocumentSymbol[]`
9+
10+
`extractDocumentSymbols` names every symbol by `node.nameText`, so a file's outline shows `let`, `let`, `const` -- the tag names, not the variables they declare -- and a template with six `<let/...>` tags is six identical `let` rows that no one can navigate or filter. The declared variable is a `var` range on the same `Tag` node, so `parsed.read(node.var)` makes `<let/count=0>` read `count` and `<const/label=x>` read `label`. The function also builds a nested walk and then flattens it into `SymbolInformation` with a `location`, ignoring the client's `hierarchicalDocumentSymbolSupport`, so the outline of a nested template has no structure and the response uses the type LSP deprecated in favour of `DocumentSymbol`. Emitting `DocumentSymbol[]` with `children` from that same `visit` recursion is close to free.
11+
12+
Check: request `textDocument/documentSymbol` on a file containing `<let/count=0>`, `<const/label="x">` and a `<div>` wrapping a `<span>`, from a client that advertises `hierarchicalDocumentSymbolSupport: true`; today the response is a flat list named `["let","const","div","span"]`, and it should be a nested `DocumentSymbol[]` named `["count","label","div"]` with `span` as `div`'s child.
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
type: bug
3+
impact: med
4+
effort: low
5+
site: packages/language-server/src/index.ts › connection.onHover
6+
---
7+
8+
# Answer a request for a document the server is not tracking with `null` instead of throwing
9+
10+
Every request handler passes `documents.get(params.textDocument.uri)!` straight into the service, and `get` returns `undefined` whenever the URI is not open and cannot be read off disk -- a non-`file:` scheme, or a path that no longer exists. `hover`, `definition`, `references`, `rename`, `documentHighlight`, `documentSymbol`, `documentColor` and `codeAction` then reject the request with a JSON-RPC internal error carrying a raw JS message: `Request textDocument/hover failed with message: Cannot read properties of undefined (reading 'uri')` for a deleted path, and the same for any `untitled:` document, which `doClose` deliberately drops from the cache. Clients race `didOpen`/`didClose` and fire requests against documents they have just closed, and the spec answer for a document the server does not have is a `null` result, not an error the user sees in the LSP output channel. Guard once at the top of each handler (or add a `documents.require` helper that returns `null`) rather than asserting with `!`.
11+
12+
Check: after `initialize`, send `textDocument/hover` for a `.marko` URI that was never opened and does not exist on disk; today the response is a JSON-RPC error whose message is `Cannot read properties of undefined (reading 'uri')`, and it should be `"result": null`.
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
type: dx
3+
impact: med
4+
effort: med
5+
site: packages/type-check/src/cli.ts › args
6+
---
7+
8+
# Let `mtc` scope a run to the files the caller named instead of always reporting the whole project
9+
10+
`args` accepts only `--project`, `--display`, `--generateTrace`, `--help` and `--version`, so there is no way to ask "does this file check". A path handed to the CLI is not rejected either: `arg` is `permissive: false` but leaves positionals in `args._`, and `run` never reads them, so `mtc src/clean.marko` silently type-checks everything and reports an unrelated file's error while exiting 1. That is the wrong shape for the loop `mtc` is used in -- an agent or a pre-commit hook checking one edited template gets a report about a file it did not touch and has to diff the run against a baseline to find its own errors. Accept one or more paths (or globs) and filter the reported diagnostics to them, or reject the positional with the usage text so the limitation is at least visible.
11+
12+
Check: in a project with a clean `src/clean.marko` and an unrelated `src/other.ts` that has an unused local, run `mtc src/clean.marko`; today it prints `src/other.ts:2:7 - error TS6133` and exits 1, and should either report nothing for the named file or refuse the argument.
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
type: bug
3+
impact: high
4+
effort: med
5+
site: packages/language-server/src/service/script/index.ts › doValidate
6+
---
7+
8+
# Offer the TypeScript quick fixes for the diagnostics the script plugin publishes
9+
10+
The script plugin publishes the full TypeScript diagnostic set out of `doValidate` -- `[script/2552] Cannot find name 'formatName'. Did you mean 'FormData'?`, `[script/6133] 'VERSION' is declared but its value is never read` -- but it implements no `doCodeActions`, so `textDocument/codeAction` answers `null` for every one of them, at the diagnostic's own range, over the whole document, with `only: ["quickfix"]` and with `only: ["source.fixAll.marko"]` alike. The server advertises `codeActionProvider` with `markoCodeActionKinds`, and that promise is kept only by `service/marko/code-actions.ts` (diagnostics that carry a Marko-compiler `fix`) and by the style plugin inside a `<style>` block, so in an ordinary `.marko` file the editor shows "No code actions available" and auto-import -- the most-used action in a TypeScript codebase -- does not exist. `ts.LanguageService.getCodeFixesAtPosition` is already reachable through the same `getTSProject`/`processScript` pair the plugin's `doComplete` and `doHover` use, and the generated-to-source mapping needed to translate the edits back is the same one `doRename` already applies.
11+
12+
Check: `didOpen` a `.marko` file that calls an un-imported export of a sibling `util.ts`, wait for the published `[script/2552]`, then request `textDocument/codeAction` at that diagnostic's range; today the result is `null` and it should contain the TypeScript "Add import from './util'" fix.
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
type: bug
3+
impact: med
4+
effort: high
5+
site: packages/language-server/src/service/marko/definition/OpenTagName.ts › OpenTagName
6+
---
7+
8+
# Answer references and rename at a custom tag name, not just go-to-definition
9+
10+
`OpenTagName` resolves a tag name to its defining file for `textDocument/definition`, but the Marko plugin exposes no `findReferences`, `findDocumentHighlights`, `prepareRename` or `doRename`, so all four return `null` at a `<user-card` tag-name position -- VS Code reports "The element can't be renamed" and "no references found" on a name that go-to-definition resolves in the same keystroke. Every other identifier in the file refactors: `prepareRename` on the `user` attribute of the same tag returns a range and `textDocument/rename` rewrites the attribute across the consumer and `src/tags/user-card/index.marko`. A tag name is the identifier whose usages are hardest to find by hand, because the declaration is a directory name and the usages are kebab-case in markup, so it is the one that most needs the server. References and highlights are the cheap half and need only the existing `lookup` plus a scan of the project's `.marko` files; a rename additionally needs a `RenameFile` in `documentChanges` for the tag directory, which clients that advertise `resourceOperations` already accept.
11+
12+
Check: `didOpen` a template containing `<user-card user={ name: "a", age: 1 }/>` beside `src/tags/user-card/index.marko` and send `textDocument/prepareRename` at the tag-name offset and at the `user` attribute offset; today the tag name returns `null` while the attribute returns a range, and the tag name should return a range whose rename rewrites every `<user-card` usage.

0 commit comments

Comments
 (0)