From 804b2baf444f0634544cf56b78befd414054cdce Mon Sep 17 00:00:00 2001 From: abose Date: Thu, 2 Jul 2026 13:21:15 +0530 Subject: [PATCH 1/3] feat(lsp): wire language-server quickfixes into the problems-panel fix workflow Diagnostics and fixes are separate LSP channels: fixes must be requested via textDocument/codeAction with the diagnostics as context. We consumed only publishDiagnostics, so the servers' quickfixes (vtsls implements them fully) never reached CodeInspection's existing fix UI (fixable icons, per-problem fix buttons, Fix All). Server-agnostic by construction - everything lives on the generic LanguageClient prototype and the per-client LintingProvider, gated purely on runtime capabilities, so any future server (PHP etc.) inherits quickfixes with zero extra wiring: - LSPClient: advertise codeAction literal support (quickfix kind; no resolveSupport, so conformant servers inline edits - one round trip). New requestCodeActions (flush -> codeAction with context.diagnostics, gated on the server's codeActionProvider, boolean or object) and resolveCodeAction (only for actions missing `edit` on servers advertising resolveProvider). publishDiagnostics hands the RAW diagnostics (range/code/data intact - the provider flattens them for CodeInspection) to the quickfix layer. - LintingProvider: idle-fetch policy so this is cheap - fetch ~800ms after diagnostics settle, rescheduled by newer publishes (zero codeAction traffic while typing), only for the active server-synced file, deduped by (lastChangeTimestamp, diagnostics signature) so identical re-publishes and file switches to unchanged files cost zero requests; an activeEditorChange hook catches diagnostics that arrived in a background pane. Strategy is runtime-adaptive: one batched whole-file request (vtsls answers it), falling back to per-diagnostic requests capped at 20. Admitted actions: literal quickfix, not disabled, exactly one TextEdit in exactly this file -> converted to CodeInspection's fix contract (replaceText + character offsets via indexFromPos) and attached to the matching cached error, then CodeInspection.requestRun() re-registers them (staleness stays automatic: CodeInspection drops fixes whenever the document changes). Identical re-publishes now also KEEP the existing errors array instead of replacing it - a fresh array would silently wipe the decorated fixes without triggering a re-fetch. Diagnostics additionally carry endPos now, so squiggles span the server-reported range instead of one token. - CodeInspection: correct the stale fix JSDoc (replace -> replaceText, which _isInvalidFix actually requires). End-to-end test in integration:TypeScript LSP: consol.log(1) -> the 2552 diagnostic appears, the idle fetch surfaces Fix All, clicking it rewrites to console.log(1). Fix application needs no editor focus, so it runs in an unfocused test window. Suite green 14/14; verified live (single fix, Fix All, icons reappearing after typing settles). --- .../default/TypeScriptSupport/unittests.js | 37 +++ src/language/CodeInspection.js | 4 +- src/languageTools/DefaultProviders.js | 232 +++++++++++++++++- src/languageTools/LSPClient.js | 75 +++++- 4 files changed, 338 insertions(+), 10 deletions(-) diff --git a/src/extensions/default/TypeScriptSupport/unittests.js b/src/extensions/default/TypeScriptSupport/unittests.js index 35b2b20d7a..28a78e896b 100644 --- a/src/extensions/default/TypeScriptSupport/unittests.js +++ b/src/extensions/default/TypeScriptSupport/unittests.js @@ -329,6 +329,43 @@ define(function (require, exports, module) { expect(content("jsconfig.json", { checkJs: true }, false).compilerOptions.checkJs).toBe(false); }); + // LSP quickfixes: diagnostics and fixes are separate channels - after diagnostics land, the + // LintingProvider idle-fetches textDocument/codeAction quickfixes, decorates the cached + // errors, and re-runs inspection so the Fix All button appears. `consol.log(1)` produces + // TS 2552 ("Cannot find name 'consol'") whose quickfix is a single same-file text edit + // ("Change spelling to 'console'") - exactly the admitted shape. Fix application goes + // through Editor.replaceMultipleRanges, so no editor focus is needed (unlike the code-hint + // menu), making this runnable in an unfocused test window. + it("wires LSP quickfixes into the problems panel Fix All workflow", async function () { + const FileSystem = testWindow.brackets.test.FileSystem; + const projectPath = await SpecRunnerUtils.getTempTestDirectory(testRootSpec + "ts"); + await jsPromise(SpecRunnerUtils.createTextFile( + path.join(projectPath, "fixable.ts"), "consol.log(1);\n", FileSystem)); + await SpecRunnerUtils.loadProjectInTestWindow(projectPath); + await awaitsForDone(SpecRunnerUtils.openProjectFiles(["fixable.ts"]), "open fixable.ts"); + + await awaitsFor(function () { + return $("#problems-panel").text().indexOf("Cannot find name 'consol'") !== -1; + }, "the spelling diagnostic to appear in the problems panel", 30000); + + // The idle codeAction fetch (~800ms after diagnostics settle) attaches the fix and + // nudges a re-run - the Fix All button becoming visible proves the whole chain. + await awaitsFor(function () { + const $btn = $("#problems-panel .problems-fix-all-btn"); + return $btn.length && !$btn.hasClass("forced-hidden"); + }, "the Fix All button to appear once quickfixes are fetched", 30000); + + const editor = EditorManager.getCurrentFullEditor(); + $("#problems-panel .problems-fix-all-btn").click(); + await awaitsFor(function () { + return editor.document.getText().indexOf("console.log(1);") !== -1; + }, "the quickfix to change consol -> console", 10000); + + await awaitsForDone(CommandManager.execute(Commands.FILE_CLOSE, { _forceClose: true }), + "close fixable.ts"); + await SpecRunnerUtils.removeTempDirectory(); + }, 90000); + // ----- hover quick-actions (Go to Definition / Find Usages) ------------------------------- // Query the hover popover at a position the same way QuickViewManager does internally. diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index 0cc3db380a..8ca27655ba 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -912,7 +912,7 @@ define(function (require, exports, module) { * htmlMessage:string, * type:?Type , * fix: { // an optional fix, if present will show the fix button - * replace: "text to replace the offset given below", + * replaceText: "text to replace the offset given below", * rangeOffset: { * start: number, * end: number @@ -929,7 +929,7 @@ define(function (require, exports, module) { * @property {string} htmlMessage - The error message to be displayed as HTML. * @property {?Type} type - The type of the error. Defaults to `Type.WARNING` if unspecified. * @property {?Object} fix - An optional fix object. - * @property {string} fix.replace - The text to replace the error with. + * @property {string} fix.replaceText - The text to replace the error with. * @property {Object} fix.rangeOffset - The range within the text to replace. * @property {number} fix.rangeOffset.start - The start offset of the range. * @property {number} fix.rangeOffset.end - The end offset of the range. diff --git a/src/languageTools/DefaultProviders.js b/src/languageTools/DefaultProviders.js index 749fe8b124..753c181905 100644 --- a/src/languageTools/DefaultProviders.js +++ b/src/languageTools/DefaultProviders.js @@ -832,6 +832,19 @@ define(function (require, exports, module) { this._promiseMap = new Map(); this._lastSignature = new Map(); this._validateOnType = false; + // --- quickfix (textDocument/codeAction) state --- + this._quickFixClient = null; // LanguageClient injected by LSPClient._registerProviders + this._rawDiagnostics = new Map(); // filePath -> raw LSP diagnostics (setInspectionResults flattens) + this._quickFixState = new Map(); // filePath -> {timestamp, signature} of the last fetch + this._quickFixTimer = null; // idle-fetch timer (cleared on every newer publish) + // Diagnostics can arrive while their file sits in a background pane (fetches only run for + // the active file) - when such a file becomes active, schedule the fetch it missed. + const self = this; + EditorManager.on("activeEditorChange", function (evt, current) { + if (current && current.document) { + self._scheduleQuickFixFetch(current.document.file._path); + } + }); } LintingProvider.prototype.setClient = setClient; @@ -843,11 +856,19 @@ define(function (require, exports, module) { this._results.delete(filePath); this._promiseMap.delete(filePath); this._lastSignature.delete(filePath); + this._rawDiagnostics.delete(filePath); + this._quickFixState.delete(filePath); } else { //clear all results this._results.clear(); this._promiseMap.clear(); this._lastSignature.clear(); + this._rawDiagnostics.clear(); + this._quickFixState.clear(); + } + if (this._quickFixTimer) { + clearTimeout(this._quickFixTimer); + this._quickFixTimer = null; } }; @@ -866,18 +887,15 @@ define(function (require, exports, module) { line: obj.range.start.line, ch: obj.range.start.character }, + endPos: { + line: obj.range.end.line, + ch: obj.range.end.character + }, message: obj.message, type: (obj.severity === 1 ? CodeInspection.Type.ERROR : (obj.severity === 2 ? CodeInspection.Type.WARNING : CodeInspection.Type.META)) }; }); - this._results.set(filePath, { - errors: errors - }); - if(this._promiseMap.get(filePath)) { - this._promiseMap.get(filePath).resolve(this._results.get(filePath)); - this._promiseMap.delete(filePath); - } // Language servers re-publish diagnostics in waves (e.g. syntax then semantic passes) and // again on every edit - frequently with identical content. Re-running inspection only to // render the same problems rebuilds the Problems panel for nothing, which both wastes work @@ -890,6 +908,19 @@ define(function (require, exports, module) { previous = this._lastSignature.has(filePath) ? this._lastSignature.get(filePath) : "[]", changed = previous !== signature; this._lastSignature.set(filePath, signature); + + // Keep the EXISTING errors array when content is unchanged: the quickfix layer decorates the + // cached errors with `fix` after an async codeAction fetch, and replacing the array on an + // identical re-publish would silently wipe those fixes without triggering a re-fetch. + if (changed || !this._results.has(filePath)) { + this._results.set(filePath, { + errors: errors + }); + } + if(this._promiseMap.get(filePath)) { + this._promiseMap.get(filePath).resolve(this._results.get(filePath)); + this._promiseMap.delete(filePath); + } if (this._validateOnType && changed) { var editor = EditorManager.getActiveEditor(), docPath = editor ? editor.document.file._path : ""; @@ -905,6 +936,193 @@ define(function (require, exports, module) { } }; + // ----- quickfixes (textDocument/codeAction) ------------------------------------------------ + // Diagnostics and their fixes are separate LSP channels: fixes must be REQUESTED per range with + // the diagnostics as context. CodeInspection needs fixes ON the errors at scan time, so the flow + // is: retain the raw diagnostics -> idle-fetch code actions (never in the typing hot path) -> + // decorate the cached errors with { replaceText, rangeOffset } -> requestRun() so CodeInspection + // re-pulls and registers them (fix icons + Fix All appear). Server-agnostic: gated purely on the + // server's codeActionProvider capability via LanguageClient.requestCodeActions. + + // Fetch this long after diagnostics settle. Typing produces a new publish per change wave, which + // reschedules - so no codeAction traffic while the user is actively typing, and a fetched fix is + // never dead-on-arrival (CodeInspection drops fixes whose document changed anyway). + var QUICKFIX_IDLE_MS = 800; + // Per-diagnostic fallback cap, for servers that don't answer a whole-file batched request. + var MAX_QUICKFIX_REQUESTS = 20; + + /** + * Record the raw diagnostics for a file and schedule an idle quickfix fetch. Called by the + * publishDiagnostics handler right after setInspectionResults. + * @param {string} filePath + * @param {Array} rawDiagnostics - unflattened LSP diagnostics (range/code/data intact) + */ + LintingProvider.prototype.updateQuickFixes = function (filePath, rawDiagnostics) { + this._rawDiagnostics.set(filePath, rawDiagnostics || []); + this._scheduleQuickFixFetch(filePath); + }; + + LintingProvider.prototype._scheduleQuickFixFetch = function (filePath) { + var self = this; + if (!this._quickFixClient || !this._rawDiagnostics.has(filePath)) { + return; + } + var editor = EditorManager.getActiveEditor(); + if (!editor || editor.document.file._path !== filePath || + !this._quickFixClient.servesDocument(editor)) { + return; // fixes are only fetched for the active, server-synced document + } + var state = this._quickFixState.get(filePath); + if (state && state.timestamp === editor.document.lastChangeTimestamp && + state.signature === this._lastSignature.get(filePath)) { + return; // already fetched for this exact document + diagnostics content + } + if (this._quickFixTimer) { + clearTimeout(this._quickFixTimer); + } + this._quickFixTimer = setTimeout(function () { + self._quickFixTimer = null; + self._fetchQuickFixes(filePath); + }, QUICKFIX_IDLE_MS); + }; + + /** + * @private + * The single admitted fix of a code action, or null. Admitted: a literal quickfix CodeAction, + * not disabled, whose WorkspaceEdit touches EXACTLY this file with EXACTLY one TextEdit (the + * CodeInspection fix contract is one contiguous same-document replace). Legacy bare Commands + * (no edit to apply client-side) are skipped. + * @return {?{replaceText:string, rangeOffset:{start:number, end:number}}} + */ + LintingProvider.prototype._fixFromAction = function (action, editor, filePath) { + if (!action || !action.title || action.disabled) { + return null; + } + if (!action.kind || action.kind.indexOf("quickfix") !== 0) { + return null; // not a quickfix (servers may ignore context.only), or a bare Command + } + var edit = action.edit; + if (!edit) { + return null; + } + var uri = null, edits = null; + if (edit.documentChanges && edit.documentChanges.length === 1 && + edit.documentChanges[0].textDocument) { + uri = edit.documentChanges[0].textDocument.uri; + edits = edit.documentChanges[0].edits; + } else if (edit.changes) { + var uris = Object.keys(edit.changes); + if (uris.length === 1) { + uri = uris[0]; + edits = edit.changes[uri]; + } + } + if (!uri || !edits || edits.length !== 1 || typeof edits[0].newText !== "string") { + return null; // multi-file / multi-edit / create-rename ops - beyond the one-replace contract + } + // Compare as decoded platform paths so URI encoding differences can't break the match. + var ownUri = this._quickFixClient.uriForPath(filePath); + if (PathConverters.uriToPath(uri) !== PathConverters.uriToPath(ownUri)) { + return null; // edit lands in a different file + } + var range = edits[0].range; + return { + replaceText: edits[0].newText, + rangeOffset: { + start: editor.indexFromPos({ line: range.start.line, ch: range.start.character }), + end: editor.indexFromPos({ line: range.end.line, ch: range.end.character }) + } + }; + }; + + // Attach an action's fix to the cached error matching one of the action's diagnostics. + // `forDiagnostic` pins the target in per-diagnostic fallback mode, where the request itself + // identifies the diagnostic even when the server omits action.diagnostics. + LintingProvider.prototype._attachFix = function (errors, action, fix, forDiagnostic) { + var diags = (action.diagnostics && action.diagnostics.length) ? action.diagnostics + : (forDiagnostic ? [forDiagnostic] : []); + for (var d = 0; d < diags.length; d++) { + var diag = diags[d]; + for (var i = 0; i < errors.length; i++) { + var err = errors[i]; + if (err.pos.line === diag.range.start.line && err.pos.ch === diag.range.start.character && + err.message === diag.message && (!err.fix || action.isPreferred)) { + err.fix = fix; + return true; + } + } + } + return false; + }; + + LintingProvider.prototype._fetchQuickFixes = async function (filePath) { + var self = this; + var client = this._quickFixClient; + var editor = EditorManager.getActiveEditor(); + var diagnostics = this._rawDiagnostics.get(filePath) || []; + var cached = this._results.get(filePath); + if (!client || !editor || editor.document.file._path !== filePath || + !client.servesDocument(editor) || !diagnostics.length || !cached) { + return; + } + var fetchedAt = editor.document.lastChangeTimestamp; + // Record the fetch attempt up front so identical re-publishes don't re-fetch, even when the + // server turns out to have no fixes for these diagnostics. + this._quickFixState.set(filePath, { + timestamp: fetchedAt, + signature: this._lastSignature.get(filePath) + }); + + try { + // One batched whole-file request first (spec-legal, one round trip for every fix)... + var lastLine = editor.lineCount() - 1; + var wholeFile = { + start: { line: 0, character: 0 }, + end: { line: lastLine, character: editor.document.getLine(lastLine).length } + }; + var actions = await client.requestCodeActions({ + filePath: filePath, range: wholeFile, diagnostics: diagnostics + }); + var attached = 0; + if (actions && actions.length) { + for (var i = 0; i < actions.length; i++) { + var resolved = await client.resolveCodeAction(actions[i]); // no-op unless deferred + var fix = this._fixFromAction(resolved, editor, filePath); + if (fix && this._attachFix(cached.errors, resolved, fix, null)) { + attached++; + } + } + } else { + // ...falling back to capped per-diagnostic requests for servers that don't answer + // the batched form. + var capped = diagnostics.slice(0, MAX_QUICKFIX_REQUESTS); + for (var d = 0; d < capped.length; d++) { + var acts = await client.requestCodeActions({ + filePath: filePath, range: capped[d].range, diagnostics: [capped[d]] + }); + for (var a = 0; acts && a < acts.length; a++) { + var res = await client.resolveCodeAction(acts[a]); + var f = this._fixFromAction(res, editor, filePath); + if (f && this._attachFix(cached.errors, res, f, capped[d])) { + attached++; + break; // one fix per diagnostic + } + } + } + } + // Re-render only when something changed AND the document didn't move under us (a moved + // document gets fresh diagnostics -> a fresh fetch; its fixes would be dropped anyway). + var activeEditor = EditorManager.getActiveEditor(); + if (attached && activeEditor && activeEditor.document.file._path === filePath && + activeEditor.document.lastChangeTimestamp === fetchedAt && + self._isRegisteredInspector(filePath)) { + CodeInspection.requestRun(); + } + } catch (err) { + console.warn("[LSP] quickfix fetch failed:", err && (err.message || err)); + } + }; + /** * @private * @param {string} filePath - active document path diff --git a/src/languageTools/LSPClient.js b/src/languageTools/LSPClient.js index ab6b964783..6b286db83e 100644 --- a/src/languageTools/LSPClient.js +++ b/src/languageTools/LSPClient.js @@ -213,6 +213,10 @@ define(function (require, exports, module) { uri: vfsUri, diagnostics: diagnostics }); + // Hand the RAW diagnostics (setInspectionResults flattens them, losing range/code/data) + // to the quickfix layer, which idle-fetches textDocument/codeAction fixes for them and + // decorates the cached results - see LintingProvider.updateQuickFixes. + client.lintingProvider.updateQuickFixes(vfsPath, diagnostics); } } @@ -531,6 +535,62 @@ define(function (require, exports, module) { return deferred.promise(); }; + /** + * Quickfix code actions for the given range's diagnostics (LSP textDocument/codeAction). + * Server-agnostic: gated on the server's codeActionProvider capability (boolean or object). + * @param {{filePath:string, range:Object, diagnostics:Array}} params - `range` is an LSP + * range ({start:{line,character}, end:{...}}) and `diagnostics` the raw LSP diagnostics to + * pass as context (servers key their fixes off these). + * @return {jQuery.Promise} resolves the server's (CodeAction|Command)[] or [] + */ + LanguageClient.prototype.requestCodeActions = function (params) { + const self = this; + const deferred = $.Deferred(); + if (!this.capabilities || !this.capabilities.codeActionProvider) { + return deferred.resolve([]).promise(); // server offers no code actions + } + (async function () { + try { + await DocumentSync.flush(self, params.filePath); + const result = await self._request("textDocument/codeAction", { + textDocument: { uri: self.uriForPath(params.filePath) }, + range: params.range, + context: { + diagnostics: params.diagnostics || [], + only: ["quickfix"] + } + }); + deferred.resolve(Array.isArray(result) ? result : []); + } catch (err) { + console.warn("[LSP] request failed:", err && (err.message || err)); + deferred.reject(err); + } + }()); + return deferred.promise(); + }; + + /** + * Fill in a CodeAction's deferred properties (typically `edit`) via codeAction/resolve. We don't + * advertise resolveSupport, so conformant servers inline edits and this is never needed - it + * exists for servers that defer anyway, gated on their codeActionProvider.resolveProvider. + * Mirrors resolveCompletion: always resolves, falling back to the unresolved action. + * @param {Object} action - the CodeAction as returned by requestCodeActions + * @return {jQuery.Promise} + */ + LanguageClient.prototype.resolveCodeAction = function (action) { + const deferred = $.Deferred(); + const provider = this.capabilities && this.capabilities.codeActionProvider; + if (!provider || !provider.resolveProvider) { + return deferred.resolve(action).promise(); + } + this._request("codeAction/resolve", action).then(function (resolved) { + deferred.resolve(resolved || action); + }, function () { + deferred.resolve(action); + }); + return deferred.promise(); + }; + // ------------------------------------------------------------------------------------------ // Server lifecycle + provider registration // ------------------------------------------------------------------------------------------ @@ -567,7 +627,17 @@ define(function (require, exports, module) { }, definition: { dynamicRegistration: false }, references: { dynamicRegistration: false }, - publishDiagnostics: { relatedInformation: false } + publishDiagnostics: { relatedInformation: false }, + codeAction: { + dynamicRegistration: false, + // Literal CodeAction support (title/kind/edit) - without this servers degrade to + // bare Commands, which carry no WorkspaceEdit we could apply. resolveSupport is + // deliberately NOT advertised: conformant servers then inline `edit` in the + // codeAction response, sparing a codeAction/resolve round-trip per fix. + codeActionLiteralSupport: { + codeActionKind: { valueSet: ["quickfix"] } + } + } }, workspace: { workspaceFolders: true, configuration: false } }; @@ -634,6 +704,9 @@ define(function (require, exports, module) { // recorded so the provider can tell whether it is still a participating inspector before nudging a // re-run on async diagnostics. client.lintingProvider._inspectionProviderName = client.serverId; + // The quickfix layer requests textDocument/codeAction through this client (capability-gated + // inside requestCodeActions), keeping the provider itself transport-agnostic. + client.lintingProvider._quickFixClient = client; client.hover = new HoverProvider.HoverProvider(client); CodeHintManager.registerHintProvider(client.codeHints, langs, DEFAULT_PRIORITY); From 2b29f14b5dd37811cceb7776ac6eabe0a90cad9f Mon Sep 17 00:00:00 2001 From: abose Date: Thu, 2 Jul 2026 13:21:24 +0530 Subject: [PATCH 2/3] fix(quickview): LSP hover content stretches to the popover width The QuickView popover centers and shrink-wraps each provider's content (.quick-view-item is a flex column with align-items:center - right for image/color previews). When another provider widens the popover - e.g. the problem row with its Fix button that now accompanies quickfixes - the narrower hover content floated centered in the leftover space (a small signature box in the middle of a wide popup). align-self:stretch on .lsp-hover-quickview makes just the LSP hover fill the available width, left-aligned, while image previews stay centered as designed. --- src/styles/brackets.less | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/styles/brackets.less b/src/styles/brackets.less index 2b8e8945c3..1eee29d54b 100644 --- a/src/styles/brackets.less +++ b/src/styles/brackets.less @@ -2221,6 +2221,11 @@ a, img { .lsp-hover-quickview { text-align: left; + // The QuickView popover centers + shrink-wraps each provider's content (.quick-view-item is a + // flex column with align-items:center - right for image/color previews). When another provider + // (e.g. a problem row with its Fix button) makes the popover wider, our content would float + // centered in the leftover space - stretch to the available width instead. + align-self: stretch; max-width: 520px; padding: 2px 4px; font-size: 12.5px; From 35be397b9644303f1eb4b49889844140a75507aa Mon Sep 17 00:00:00 2001 From: abose Date: Thu, 2 Jul 2026 14:09:29 +0530 Subject: [PATCH 3/3] fix(lsp): jump-to-definition hung forever when the server had no answer The intermittent CI failure of "should jump to definition" (LegacyInteg: EditorCommandHandlers) was not cold-start latency: the spec's first NAVIGATE_JUMPTO_DEFINITION fired while tsserver was still loading the project, the server answered with no definition, and doJumpToDef's deferred only resolved inside `if (msgObj && msgObj.range)` - an empty result NEVER settled it. The command promise then hung forever, and awaitsFor races each poll invocation against the FULL timeout, so the "retry loop" never retried once and the spec burned its whole budget on one dead promise (the SpecRunner.js:141 stack in the CI logs). Manual jumps worked the whole time - the server was healthy; the spec was pinned. - DefaultProviders.doJumpToDef: settle (reject) when the server returns no definition, and on a failed cross-file FILE_OPEN. Also fixes real users: F12 on a symbol with no definition left a permanently pending command. - src-node/lsp-client: on unexpected server exit, reject all in-flight requests immediately - the dead process can never answer them, and they previously hung callers until the 2-minute per-request timeout (only graceful stops flushed the pending map). - The spec: the one-time tsserver cold start now happens in the suite's beforeAll warm-up (the TypeScript LSP suite's pattern, 90s budget there); the spec itself waits max 15s, polling every 500ms with each jump attempt time-boxed to 3s - a slow or rejected attempt is a retry, never a pin - and a rejected jump no longer aborts awaitsFor via an exception. LegacyInteg:EditorCommandHandlers Integration green 12/12 twice in a row (including the focus-sensitive clipboard spec). --- src-node/lsp-client.js | 6 +++ src/languageTools/DefaultProviders.js | 8 +++ test/spec/EditorCommandHandlers-integ-test.js | 51 ++++++++++++++++--- 3 files changed, 58 insertions(+), 7 deletions(-) diff --git a/src-node/lsp-client.js b/src-node/lsp-client.js index f8929ec9f2..02800d35d4 100644 --- a/src-node/lsp-client.js +++ b/src-node/lsp-client.js @@ -218,6 +218,12 @@ exports.startServer = async function startServer(params) { if (code) { console.error(`[lsp-client][${serverId}] exited code=${code} signal=${signal || 'none'}`); } + // Fail every in-flight request immediately - the dead process can never answer them, and + // leaving them pending stalls callers until the per-request timeout (2 minutes). + for (const { reject: rejectPending } of serverState.pending.values()) { + rejectPending(new Error(`Server ${serverId} exited with pending request`)); + } + serverState.pending.clear(); nodeConnector.triggerPeer('serverExit', { serverId, code, signal, stderr }); if (!hasResolved) { hasResolved = true; diff --git a/src/languageTools/DefaultProviders.js b/src/languageTools/DefaultProviders.js index 753c181905..f8b0f1ee28 100644 --- a/src/languageTools/DefaultProviders.js +++ b/src/languageTools/DefaultProviders.js @@ -814,11 +814,19 @@ define(function (require, exports, module) { .done(function () { setJumpPosition(startCurPos); $deferredHints.resolve(); + }) + .fail(function () { + $deferredHints.reject(); }); } else { //definition is in current document setJumpPosition(startCurPos); $deferredHints.resolve(); } + } else { + // No definition at this position (servers answer null/[] - e.g. tsserver while it + // is still loading the project). MUST settle: an unresolved deferred here leaves + // the NAVIGATE_JUMPTO_DEFINITION command promise pending forever. + $deferredHints.reject(); } }).fail(function () { $deferredHints.reject(); diff --git a/test/spec/EditorCommandHandlers-integ-test.js b/test/spec/EditorCommandHandlers-integ-test.js index 4b33190445..7a0489db0b 100644 --- a/test/spec/EditorCommandHandlers-integ-test.js +++ b/test/spec/EditorCommandHandlers-integ-test.js @@ -115,7 +115,27 @@ define(function (require, exports, module) { EditorManager = testWindow.brackets.test.EditorManager; await SpecRunnerUtils.loadProjectInTestWindow(testPath); - }, 30000); + + if (testWindow.Phoenix.isNativeApp) { + // Warm up the TypeScript language server here, where the one-time cold start (spawn + // vtsls + tsserver loading the TypeScript library and project) belongs - so the + // jump-to-definition spec below only needs a short wait for its own request instead + // of budgeting for a cold server (its very first request could otherwise pend longer + // than any reasonable spec timeout on slow/loaded CI runners). + await awaitsForDone( + CommandManager.execute(Commands.CMD_ADD_TO_WORKINGSET_AND_OPEN, + {fullPath: testPath + "/test.js"}), + "warm-up: open test.js"); + const LSPClient = await new Promise(function (resolve) { + testWindow.brackets.getModule(["languageTools/LSPClient"], resolve); + }); + await awaitsFor(function () { + return LSPClient.isLintingProviderActive("javascript"); + }, "the TypeScript language server to finish its cold start", 90000); + await awaitsForDone(CommandManager.execute(Commands.FILE_CLOSE_ALL, { _forceClose: true }), + "close warm-up file"); + } + }, 120000); afterAll(async function () { await closeTestWindow(); @@ -202,17 +222,34 @@ define(function (require, exports, module) { // JavaScript at a higher priority than the built-in Tern provider. vtsls returns // testMe's full declaration range and we jump to its start (the `function` // keyword), giving a collapsed cursor at {0,0} - whereas Tern selects the - // identifier name. The server can take a moment to prime after the file opens, so - // retry the jump until the LSP result lands rather than using a fixed wait (which - // flakes on slow CI: before vtsls is ready, Tern answers with {0,9}-{0,15}). + // identifier name. The server was already warmed up in beforeAll, so 15s is + // plenty here. Each attempt is time-boxed: a slow or unanswered request counts + // as a retry instead of pinning the whole wait (awaitsFor races each pollFn + // invocation against the FULL timeout, so one hung attempt would otherwise eat + // the entire budget), and a rejected jump ("no definition yet") also retries - + // awaitsFor aborts outright on a pollFn exception, so the promise is absorbed. await awaitsFor(async function () { myEditor.setCursorPos({line: 5, ch: 8}); - await awaitsForDone(CommandManager.execute(Commands.NAVIGATE_JUMPTO_DEFINITION), - "Jump To Definition"); + var landed = await new Promise(function (resolve) { + var settled = false; + function settle(val) { + if (!settled) { + settled = true; + resolve(val); + } + } + CommandManager.execute(Commands.NAVIGATE_JUMPTO_DEFINITION) + .done(function () { settle(true); }) + .fail(function () { settle(false); }); + setTimeout(function () { settle(false); }, 3000); + }); + if (!landed) { + return false; + } var sel = myEditor.getSelection(); return sel.start.line === 0 && sel.start.ch === 0 && sel.end.line === 0 && sel.end.ch === 0; - }, "LSP jump-to-definition to land on the testMe declaration", 30000); + }, "LSP jump-to-definition to land on the testMe declaration", 15000, 500); selection = myEditor.getSelection(); expect(fixSel(selection)).toEql(fixSel({ start: {line: 0, ch: 0},