Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Empty whole-declaration interpolations currently shift all subsequent source mappings, and the new globalStyle integration lacks real-tsserver coverage.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/configuration/plugin-configuration.ts:52
- The new default
globalStyletag is only checked as configuration data. The real-tsserver fixture overridestagswithoutglobalStyle, and the styled-components syntax matrix does not exercise it, so the advertised host behavior can regress unnoticed. AddglobalStyleto the fixture configuration and syntax scenario.
CHANGELOG.md:173 - Correct the misspelling in this changelog entry.
- Files reviewed: 99/107 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The broad modernization and an unresolved E2E completion-response contract issue require human review.
Review details
Suppressed comments (3)
src/api.ts:4
- The public API comment is missing “be”, so it currently reads ungrammatically. Please change it to “allows the language service to be consumed by other libraries.”
test/e2e/tsserver-fixture/index.ts:40 translateCompletionEntryintentionally omitsreplacementSpanwhen an LSP completion has notextEdit(seetest/unit/template-language-service.test.ts:108-117), but this protocol model requires the field for every completion. Any E2E assertion that readsitem.replacementSpanfor such a valid response will be unsound or throw; make the field optional to match tsserver responses.
test/unit/tsserver-message-reader.test.ts:29- This test sends both complete frames in one chunk, so it verifies UTF-8 byte length but not the advertised case where a response is split inside a multibyte character. Split the first frame at a byte within
颜色and assert that no message is emitted until the remaining bytes arrive, so regressions in raw-byte buffering are caught.
- Files reviewed: 99/107 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The E2E harness has unresolved request-matching and bounded-shutdown issues.
Review details
Suppressed comments (2)
test/e2e/scenarios/tsserver-test-helpers.ts:27
- The harness tracks
request_seqonly to clear the pending set, but this helper still selects the first response for a command by arrival order. Scenarios such as the lifecycle and code-fix tests issue multiple requests of the same command, so an out-of-order tsserver response can be asserted against the wrong request; retain each returned sequence number and select responses byrequest_seq(or expose a request-specific helper) to make the claimed response matching reliable.
test/e2e/tsserver-fixture/index.ts:200 - When any request is still pending,
close()marks the server closed but deliberately does not callshutdown(). If tsserver drops a response or remains alive without answering, thecloseevent can never occur, so the pending-request check below is never reached and the test hangs until Vitest's timeout instead of reporting the incomplete request. Add a bounded close timeout/kill path (or otherwise force an exit and reject with the pending sequence numbers).
- Files reviewed: 100/108 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
src/features/diagnostics.ts must honor validate: false before approval.
Review details
Suppressed comments (1)
src/features/diagnostics.ts:25
validateis normalized and updated by the configuration manager, but this feature never consults it before callingdoValidation. As a result,validate: falsestill produces CSS diagnostics (the newplugin-lifecyclescenario attest/e2e/scenarios/plugin-lifecycle.test.ts:69will fail), and configuration changes cannot disable validation. Pass the configuration manager or a validation predicate into this feature and return an empty list when validation is disabled.
- Files reviewed: 100/108 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Pull request overview
Copilot reviewed 100 out of 108 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/index.ts:1
- This export is not valid TS/ESM syntax (
export { x as 'string' }is not allowed), and it also risks breaking the tsserver plugin loader contract + mismatchingtypes/index.d.cts(which declaresexport = createPlugin). Use a valid export shape and keep runtime + typings aligned (e.g.,export default createPlugin+ updateindex.d.ctsaccordingly, or provide a dedicated CJS shim entrypoint inexports.requirethat setsmodule.exports = createPlugin).
src/template-language-service.ts:1 - Constructor overload signatures must be terminated (typically with
;) and cannot appear as bareconstructor(...)declarations without a body. As written, this will not parse/typecheck. Add proper overload signature terminators (or remove overloads and use a single signature with discriminated arguments).
test/package-api/runtime.ts:1 - The code calls
pluginFactory(...)before verifying thatpluginFactoryis actually a function. If the export shape changes (which is likely given the ESM/CJS interop changes in this PR), this will throw a generic runtime TypeError before the intended diagnostic. Checktypeof pluginFactory === 'function'first, then invoke it.
Hi @quantizor, I have prepared this update to bring the project up to date while preserving its existing purpose and public API.
This PR keeps the project's role as a cross-editor TypeScript Server plugin. The work is intended to make the plugin easier to maintain, test, and publish, while also addressing several compatibility and language-service issues found during the update.
What changed
require(ESM)interoperability.globalStyleandkeyframestemplates, completion edit ranges, cache invalidation, and disabled validation. Completion translation now preserves insertion text, snippet semantics, filtering text, and optional replacement spans from the CSS language service.unknownAtRules.request_seq, and failing tests when tsserver exits unsuccessfully or leaves requests incomplete.injectGlobal, and disabled Emmet behavior.prepackbuild so a clean Git checkout cannot publish an empty shell package. Package tests remove existing build output, create and unpack the tarball, synchronously load the tsserver entry, import the ESM API, and exercise the legacy code-fix API from the packed artifact../apiexport there. The same formatting, linting, type-checking, test, package API, and package-content checks remain available locally.Compatibility notes
Because this update moves the package to TypeScript 6 and an ESM-only distribution, it is intended for a new major release. The package requires TypeScript 6.x and Node.js 22.12 or later so that tsserver hosts can synchronously load the ESM entry point. Development and builds use Node.js 24.21, while the compiled plugin continues to be tested on the Node.js 22.12 runtime minimum. VS Code with workspace TypeScript is the primary verified host; other tsserver-compatible editors should be listed as supported only after host-specific validation.
The public language-service API remains available through the
./apiexport. Legacy three-argument code-fix calls remain supported, custom virtual-document providers can opt into cache reuse, andResolvedStyledPluginConfigurationis available for consumers that need the manager's fully populated configuration. The tsserver root entry now provides condition-specific ESM and CommonJS declarations, and package-level tests verify ESM API consumption, CommonJS TypeScript consumption, and synchronous runtime loading from the packed artifact.Verification
The repository now provides a single
yarn verifyworkflow covering formatting, linting, strict type checking, 91 unit tests, 105 real-tsserver end-to-end tests, clean-output package API checks, packed-artifact runtime consumption, andnpm pack --dry-run. CI builds once on Node.js 24.21 and verifies the resulting artifact on Node.js 22.12 with TypeScript 6.0.2, Node.js 22.12 with the current TypeScript 6 release, and Node.js 24.21 with the current TypeScript 6 release. Performance benchmarks are also included to track completion latency and retained heap behavior.Next steps
After this PR, I plan to address the currently open issues in this repository. Once those have been handled, I plan to update and maintain the downstream styled-components/vscode-styled-components repository.
Please merge this PR. If you have a different direction in mind or would like any part of the update adjusted, please let me know and I will update it accordingly.
Thank you for your time and review.