From a35f1dc46d041a9937ca4526d6ba78e3a762cd06 Mon Sep 17 00:00:00 2001 From: fi3ework Date: Wed, 5 Aug 2026 21:52:02 +0800 Subject: [PATCH 1/7] feat: per-stack restart commands and hover-only status bar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restart is now a shell concern with two levels: rstack.restart ("Relaunch Extension") tears down every controller, re-runs detection and registers again from scratch; the new rstack..restart commands do the same for a single stack. The Rslint stack's own LSP-only restart is removed — bouncing just the server kept the controller's stale package resolution and version check, which is exactly what these commands exist to clear. Its settings listener now routes through the shell command instead. The status bar QuickPick is gone: the hover card is the single surface. It renders all six rows in one table (aligned state/label/action columns), colours state icons with theme variables, moves state text into the icon's native tooltip, and stacks the three global actions under a divider. Clicking the item opens the extension log directly. Unit tests cover the per-stack restart scope and the manifest contract (rstack.restart stays palette-unconditional; per-stack restarts gate on their context keys). --- packages/vscode/AGENTS.md | 2 + packages/vscode/README.md | 2 +- packages/vscode/package.json | 33 +- packages/vscode/rstest.config.mts | 8 + packages/vscode/src/detection.test.ts | 77 +++- packages/vscode/src/detection.ts | 11 +- packages/vscode/src/extension.test.ts | 380 ++++++++++++++++++ packages/vscode/src/extension.ts | 160 +++++++- packages/vscode/src/stacks/lint/index.ts | 52 +-- packages/vscode/src/statusBar.ts | 200 ++++++--- packages/vscode/tests/e2e/suite/shell.test.ts | 55 ++- 11 files changed, 822 insertions(+), 158 deletions(-) create mode 100644 packages/vscode/src/extension.test.ts diff --git a/packages/vscode/AGENTS.md b/packages/vscode/AGENTS.md index 23c1b18..c1abf10 100644 --- a/packages/vscode/AGENTS.md +++ b/packages/vscode/AGENTS.md @@ -19,6 +19,8 @@ One extension replacing the standalone `rstack.rslint` and `rstack.rstest` exten - One stack failing to register or crashing must never take another stack (or the shell) down. - The shell always activates; per-folder config detection decides which stacks start, and re-runs on config/lockfile changes without a window reload. Enable-settings are coarse kill switches only. +- Reconciles and restarts share one serialized queue (`enqueue`); a reconcile leaves a live stack alone, so the restart commands are the only path that rebuilds one. Do not add a second queue. +- Restart is a shell concern, not a stack one: `rstack.restart` rebuilds every controller, `rstack..restart` rebuilds one. A stack must never register its own restart command — a shallower "bounce the tool's process" restart keeps that controller's stale package resolution and version check, which is the bug the command exists to clear. - Deprecated `rslint.json` / `rslint.jsonc` are unsupported by decision, not omission — never make them detection signals. - Never share a child process across stacks: the tools have incompatible cwd semantics (lint LSP anchors on spawn cwd; test worker pins to project root; `rs fmt` resolves config from spawn cwd with no upward walk). - In Restricted Mode (workspace trust), only the status bar runs — no process spawns, no project code loaded. diff --git a/packages/vscode/README.md b/packages/vscode/README.md index 4c356ef..c0499ca 100644 --- a/packages/vscode/README.md +++ b/packages/vscode/README.md @@ -26,7 +26,7 @@ The extension activates on startup, then decides **per workspace folder** which | Rstest | `rstest.config.{mjs,ts,js,cjs,mts,cts}` (configurable) or `rstack.config.*` | | rstack-cli | `rstack.config.*` or `node_modules/.bin/rs` | -Config files and lockfiles are watched, so detection re-runs without a window reload. Deprecated `rslint.json` / `rslint.jsonc` configs are **not** detection signals — migrate them with `rslint --init`. +Config files and lockfiles are watched, so detection re-runs without a window reload. When something changes that none of those files record — a reinstall that leaves the lockfile untouched, or a `node_modules` that ends up broken — run **Rstack: Relaunch Extension** from the Command Palette (also on the status bar hover) to tear every tool down and start over. To rebuild a single tool, use **Rstack: Restart Rslint** / **Restart Rstest** / **Restart rs fmt**. Deprecated `rslint.json` / `rslint.jsonc` configs are **not** detection signals — migrate them with `rslint --init`. ## Supported package versions diff --git a/packages/vscode/package.json b/packages/vscode/package.json index 380ae84..65532df 100644 --- a/packages/vscode/package.json +++ b/packages/vscode/package.json @@ -43,16 +43,17 @@ }, "contributes": { "commands": [ - { - "command": "rstack.showMenu", - "title": "Show Menu", - "category": "Rstack" - }, { "command": "rstack.showOutput", "title": "Show Extension Log", "category": "Rstack" }, + { + "command": "rstack.restart", + "title": "Relaunch Extension", + "category": "Rstack", + "icon": "$(debug-restart)" + }, { "command": "rstack.migrateSettings", "title": "Migrate Rslint/Rstest Settings", @@ -65,7 +66,7 @@ }, { "command": "rstack.rslint.restart", - "title": "Restart Rslint Language Server", + "title": "Restart Rslint", "category": "Rstack", "icon": "$(refresh)" }, @@ -74,6 +75,12 @@ "title": "Show Rstest Log", "category": "Rstack" }, + { + "command": "rstack.rstest.restart", + "title": "Restart Rstest", + "category": "Rstack", + "icon": "$(refresh)" + }, { "command": "rstack.rstest.updateSnapshot", "title": "Update Snapshot", @@ -107,6 +114,12 @@ "command": "rstack.fmt.output.focus", "title": "Show rs fmt Log", "category": "Rstack" + }, + { + "command": "rstack.fmt.restart", + "title": "Restart rs fmt", + "category": "Rstack", + "icon": "$(refresh)" } ], "configuration": [ @@ -347,10 +360,18 @@ "command": "rstack.rstest.output.focus", "when": "rstack.rstest.active" }, + { + "command": "rstack.rstest.restart", + "when": "rstack.rstest.active" + }, { "command": "rstack.fmt.output.focus", "when": "rstack.fmt.active" }, + { + "command": "rstack.fmt.restart", + "when": "rstack.fmt.active" + }, { "command": "rstack.rstest.updateSnapshot", "when": "false" diff --git a/packages/vscode/rstest.config.mts b/packages/vscode/rstest.config.mts index bb76e16..7ea13c6 100644 --- a/packages/vscode/rstest.config.mts +++ b/packages/vscode/rstest.config.mts @@ -10,5 +10,13 @@ export default defineConfig({ externals: { vscode: 'commonjs vscode', }, + // An externalized dependency is imported by the chunk itself, so it loads + // before any `rs.mock` can intervene — and `vscode-languageclient/node` + // does a bare `require('vscode')` at load time, which no mock can serve + // from plain Node. Bundling it routes that require through the bundler, + // where the `vscode` external (and therefore the mock) applies. This is + // what lets a test import the shell, whose module graph reaches the Rslint + // stack. + bundleDependencies: ['vscode-languageclient'], }, }); diff --git a/packages/vscode/src/detection.test.ts b/packages/vscode/src/detection.test.ts index 8469048..8811706 100644 --- a/packages/vscode/src/detection.test.ts +++ b/packages/vscode/src/detection.test.ts @@ -1,17 +1,51 @@ import { describe, expect, it, rs } from '@rstest/core'; +import type vscode from 'vscode'; +import type { DetectionSnapshot } from './types'; // `detection.ts` imports the `vscode` namespace for the watcher/`findFiles` // paths. `detectionWatchPatterns` is pure, but the module still has to load, so // the namespace is stubbed away: unit tests run in plain Node, with no -// extension host (unit tests are Rstest, E2E is Electron). +// extension host (unit tests are Rstest, E2E is Electron). The stub carries +// exactly what `DetectionService` touches with no workspace folder open — its +// event plumbing — so the notification rules can be exercised here too. rs.mock('vscode', () => { - const vscode = {}; + class EventEmitter { + readonly #listeners = new Set<(value: unknown) => void>(); + readonly event = (listener: (value: unknown) => void) => { + this.#listeners.add(listener); + return { + dispose: () => { + this.#listeners.delete(listener); + }, + }; + }; + fire(value: unknown): void { + for (const listener of [...this.#listeners]) { + listener(value); + } + } + dispose(): void { + this.#listeners.clear(); + } + } + const disposable = { dispose: () => undefined }; + const vscode = { + EventEmitter, + workspace: { + // No folder is open: every pass produces the empty snapshot, so the + // detection signature is unchanged by construction. + workspaceFolders: undefined, + onDidChangeWorkspaceFolders: () => disposable, + onDidChangeConfiguration: () => disposable, + }, + }; return { ...vscode, default: vscode }; }); import { DEFAULT_RSTEST_CONFIG_GLOBS, DETECTION_WATCH_NAMES, + DetectionService, detectionWatchPatterns, } from './detection'; @@ -160,3 +194,42 @@ describe('detectionWatchPatterns', () => { } }); }); + +/** + * With no workspace folder open every pass yields the empty snapshot, so the + * detection signature is identical across passes by construction — exactly the + * shape a lockfile write or a `rstack.restart` produces in a real workspace + * whose `node_modules` was replaced without touching a watched file. + */ +describe('DetectionService — notification rules', () => { + const fakeOutput = () => + ({ + info: () => undefined, + warn: () => undefined, + error: () => undefined, + }) as unknown as vscode.LogOutputChannel; + + const listen = (service: DetectionService) => { + const seen: DetectionSnapshot[] = []; + service.onDidChange((snapshot) => seen.push(snapshot)); + return seen; + }; + + it('stays quiet when the signature did not change', async () => { + const service = new DetectionService(fakeOutput()); + const seen = listen(service); + await service.initialize(); + await service.refresh(); + expect(seen).toHaveLength(0); + service.dispose(); + }); + + it('does not notify after disposal', async () => { + const service = new DetectionService(fakeOutput()); + const seen = listen(service); + await service.initialize(); + service.dispose(); + await service.refresh(); + expect(seen).toHaveLength(0); + }); +}); diff --git a/packages/vscode/src/detection.ts b/packages/vscode/src/detection.ts index fc07227..d765af5 100644 --- a/packages/vscode/src/detection.ts +++ b/packages/vscode/src/detection.ts @@ -240,7 +240,9 @@ export class DetectionService implements vscode.Disposable { // out identical while every project-resolved package (Rslint binary, Rstest // core, the rstack shim) may now resolve differently. Such a pass must // notify subscribers even when the signature is unchanged, or failed - // resolutions are never retried until a window reload. + // resolutions are never retried until a window reload. Set by the lockfile + // watcher only — a caller that drives the rebuild itself does not need the + // event, it already has the fresh snapshot. #notifyUnchanged = false; #watchers: vscode.Disposable[] = []; #debounce: ReturnType | undefined; @@ -301,12 +303,15 @@ export class DetectionService implements vscode.Disposable { // Virtual filesystems cannot host a project-local toolchain. (folder) => folder.uri.scheme === 'file', ); + // Consumed before the first `await`: a pass that rejects (a folder removed + // mid-scan, a filesystem provider erroring) must not leave the flag set for + // an unrelated later pass to act on. + const notifyUnchanged = this.#notifyUnchanged; + this.#notifyUnchanged = false; const detections = await Promise.all(folders.map(detectFolder)); const snapshot = new Snapshot(detections); const signature = signatureOf(snapshot); this.#snapshot = snapshot; - const notifyUnchanged = this.#notifyUnchanged; - this.#notifyUnchanged = false; if (signature !== this.#signature || notifyUnchanged) { this.#signature = signature; this.log(snapshot); diff --git a/packages/vscode/src/extension.test.ts b/packages/vscode/src/extension.test.ts new file mode 100644 index 0000000..b8ac7e9 --- /dev/null +++ b/packages/vscode/src/extension.test.ts @@ -0,0 +1,380 @@ +import { afterEach, beforeEach, describe, expect, it, rs } from '@rstest/core'; +import type vscode from 'vscode'; + +/** + * The shell itself is the unit under test: `activate()` runs for real and the + * commands it contributes are invoked through the recorded command registry, + * the way VS Code would invoke them. Everything around it is stubbed — the + * `vscode` namespace, detection, and the three stack factories — because unit + * tests run in plain Node with no extension host (unit tests are Rstest, E2E is + * Electron, and E2E stays the ground truth for editor behaviour). + */ +interface FakeController { + register(): Promise>; + dispose(): Promise; +} + +const harness = rs.hoisted(() => { + const state = { + /** Stacks detection currently reports as detected. */ + detected: new Set(), + /** Stacks whose controller rejects in `register` / `dispose`. */ + failRegister: new Set(), + failDispose: new Set(), + /** Ordered `register:` / `dispose:` trace. */ + events: [] as string[], + /** One entry per detection pass the shell asked for. */ + refreshes: 0, + /** Everything the shell wrote to its own output channel. */ + shellLog: [] as string[], + commands: new Map unknown>(), + contextKeys: new Map(), + controller(stack: string): FakeController { + return { + register: async () => { + state.events.push(`register:${stack}`); + if (state.failRegister.has(stack)) { + throw new Error(`${stack} refuses to register`); + } + return { stack }; + }, + dispose: async () => { + state.events.push(`dispose:${stack}`); + if (state.failDispose.has(stack)) { + throw new Error(`${stack} refuses to dispose`); + } + }, + }; + }, + }; + return state; +}); + +rs.mock('vscode', () => { + class EventEmitter { + readonly #listeners = new Set<(value: unknown) => void>(); + readonly event = (listener: (value: unknown) => void) => { + this.#listeners.add(listener); + return { + dispose: () => { + this.#listeners.delete(listener); + }, + }; + }; + fire(value: unknown): void { + for (const listener of [...this.#listeners]) { + listener(value); + } + } + dispose(): void { + this.#listeners.clear(); + } + } + const disposable = { dispose: () => undefined }; + const createOutputChannel = (name: string) => { + // Only the shell channel is recorded — the failure reports the user is + // supposed to find live there. + const record = (level: string, message: string) => { + if (name === 'Rstack') { + harness.shellLog.push(`${level}: ${message}`); + } + }; + return { + name, + info: (message: string) => record('info', message), + warn: (message: string) => record('warn', message), + error: (message: string) => record('error', message), + show: () => undefined, + dispose: () => undefined, + }; + }; + const vscode = { + EventEmitter, + StatusBarAlignment: { Left: 1, Right: 2 }, + ThemeColor: class { + constructor(readonly id: string) {} + }, + MarkdownString: class { + value = ''; + isTrusted = false; + appendMarkdown(text: string): this { + this.value += text; + return this; + } + }, + window: { + createOutputChannel, + createStatusBarItem: () => ({ + name: '', + text: '', + tooltip: undefined as unknown, + command: '', + backgroundColor: undefined as unknown, + show: () => undefined, + hide: () => undefined, + dispose: () => undefined, + }), + showInformationMessage: async () => undefined, + }, + commands: { + registerCommand: ( + command: string, + handler: (...args: unknown[]) => unknown, + ) => { + harness.commands.set(command, handler); + return { + dispose: () => { + harness.commands.delete(command); + }, + }; + }, + executeCommand: async (command: string, ...args: unknown[]) => { + if (command === 'setContext') { + harness.contextKeys.set(String(args[0]), Boolean(args[1])); + return undefined; + } + const handler = harness.commands.get(command); + if (!handler) { + throw new Error(`unknown command: ${command}`); + } + return handler(...args); + }, + }, + workspace: { + isTrusted: true, + workspaceFolders: [], + getConfiguration: () => ({ + get: (_key: string, fallback?: unknown) => fallback, + }), + onDidChangeConfiguration: () => disposable, + onDidChangeWorkspaceFolders: () => disposable, + onDidGrantWorkspaceTrust: () => disposable, + }, + }; + return { ...vscode, default: vscode }; +}); + +rs.mock('./detection', () => { + const snapshot = () => ({ + folders: [], + isDetected: (stack: string) => harness.detected.has(stack), + foldersFor: () => [], + forFolder: () => undefined, + }); + class DetectionService { + readonly onDidChange = () => ({ dispose: () => undefined }); + get snapshot() { + return snapshot(); + } + async initialize() { + return this.snapshot; + } + async refresh() { + harness.refreshes += 1; + return this.snapshot; + } + dispose() {} + } + return { DetectionService }; +}); + +rs.mock('./stacks/lint', () => ({ + createRslintController: () => harness.controller('rslint'), +})); +rs.mock('./stacks/test', () => ({ + createRstestController: () => harness.controller('rstest'), +})); +rs.mock('./stacks/fmt', () => ({ + createFmtController: () => harness.controller('fmt'), +})); +rs.mock('./migration', () => ({ + maybePromptForMigration: async () => undefined, + runSettingsMigration: async () => undefined, +})); + +import { activate, deactivate } from './extension'; + +const context = { subscriptions: [] } as unknown as vscode.ExtensionContext; + +const stacksOf = (kind: 'register' | 'dispose'): string[] => + harness.events + .filter((event) => event.startsWith(`${kind}:`)) + .map((event) => event.slice(kind.length + 1)) + .sort(); + +const phasesOf = (): string[] => + harness.events.map((event) => event.split(':')[0]); + +const run = async (command: string): Promise => { + const handler = harness.commands.get(command); + if (!handler) { + throw new Error(`${command} is not registered`); + } + await handler(); +}; + +const restart = (): Promise => run('rstack.restart'); + +describe('the shell restart command', () => { + beforeEach(async () => { + harness.detected = new Set(['rslint', 'rstest', 'fmt']); + harness.failRegister.clear(); + harness.failDispose.clear(); + harness.events.length = 0; + harness.refreshes = 0; + harness.shellLog.length = 0; + harness.commands.clear(); + harness.contextKeys.clear(); + await activate(context); + expect(stacksOf('register')).toEqual(['fmt', 'rslint', 'rstest']); + harness.events.length = 0; + harness.refreshes = 0; + harness.shellLog.length = 0; + }); + + afterEach(async () => { + await deactivate(); + }); + + it('is contributed unconditionally, so it is reachable with no stack active', () => { + expect(harness.commands.has('rstack.restart')).toBe(true); + + // The command exists for the state where nothing is active, so the palette + // must offer it then. VS Code hides a contributed command from the palette + // only through a `contributes.menus.commandPalette` entry whose `when` is + // false — every other `rstack.*` command has one, so the guard here is the + // *absence* of an entry, which is exactly the kind of thing a later edit + // adds back by symmetry. + const manifest = require('../package.json') as { + contributes: { + commands: Array<{ command: string }>; + menus: { commandPalette: Array<{ command: string; when: string }> }; + }; + }; + expect( + manifest.contributes.commands.map((entry) => entry.command), + ).toContain('rstack.restart'); + expect( + manifest.contributes.menus.commandPalette.map((entry) => entry.command), + ).not.toContain('rstack.restart'); + + // The per-stack ones are the opposite: they only make sense for a stack + // that is up, so each is gated on its own context key. + for (const stack of ['rslint', 'rstest', 'fmt']) { + expect(harness.commands.has(`rstack.${stack}.restart`)).toBe(true); + expect( + manifest.contributes.menus.commandPalette.find( + (entry) => entry.command === `rstack.${stack}.restart`, + )?.when, + ).toBe(`rstack.${stack}.active`); + } + }); + + it('rebuilds only the named stack on rstack..restart', async () => { + await run('rstack.rstest.restart'); + + expect(stacksOf('dispose')).toEqual(['rstest']); + expect(stacksOf('register')).toEqual(['rstest']); + expect(phasesOf()).toEqual(['dispose', 'register']); + // Detection is global and cheap, and the point of the command is that a + // stale resolution gets redone — so it re-runs even for a single stack. + expect(harness.refreshes).toBe(1); + expect(harness.contextKeys.get('rstack.rstest.active')).toBe(true); + }); + + it('leaves a single-stack restart to the gate, same as a full one', async () => { + harness.detected.delete('rstest'); + + await run('rstack.rstest.restart'); + + expect(stacksOf('dispose')).toEqual(['rstest']); + expect(stacksOf('register')).toEqual([]); + expect(harness.contextKeys.get('rstack.rstest.active')).toBe(false); + }); + + it('disposes every registered stack and rebuilds the ones that still pass the gate', async () => { + harness.detected.delete('fmt'); + + await restart(); + + expect(stacksOf('dispose')).toEqual(['fmt', 'rslint', 'rstest']); + expect(stacksOf('register')).toEqual(['rslint', 'rstest']); + // A full reset: nothing is rebuilt before everything is torn down. + expect(phasesOf()).toEqual([ + 'dispose', + 'dispose', + 'dispose', + 'register', + 'register', + ]); + expect(harness.contextKeys.get('rstack.fmt.active')).toBe(false); + expect(harness.contextKeys.get('rstack.rslint.active')).toBe(true); + }); + + it('re-runs detection before rebuilding', async () => { + await restart(); + expect(harness.refreshes).toBe(1); + }); + + it('rebuilds a stack whose detection did not move at all', async () => { + // The whole point of the command: nothing observable changed, yet every + // controller is replaced, because `node_modules` may have been. + await restart(); + expect(stacksOf('dispose')).toEqual(['fmt', 'rslint', 'rstest']); + expect(stacksOf('register')).toEqual(['fmt', 'rslint', 'rstest']); + }); + + it('restarts the other stacks when one throws while disposing', async () => { + harness.failDispose.add('rslint'); + + await restart(); + + expect(stacksOf('dispose')).toEqual(['fmt', 'rslint', 'rstest']); + expect(stacksOf('register')).toEqual(['fmt', 'rslint', 'rstest']); + expect( + harness.shellLog.some( + (line) => + line.startsWith('error:') && + line.includes('Rslint failed to dispose'), + ), + ).toBe(true); + }); + + it('restarts the other stacks when one throws while registering', async () => { + harness.failRegister.add('rstest'); + + await restart(); + + expect(stacksOf('register')).toEqual(['fmt', 'rslint', 'rstest']); + expect(harness.contextKeys.get('rstack.rstest.active')).toBe(false); + expect(harness.contextKeys.get('rstack.rslint.active')).toBe(true); + expect(harness.contextKeys.get('rstack.fmt.active')).toBe(true); + expect( + harness.shellLog.some( + (line) => + line.startsWith('error:') && + line.includes('Rstest failed to register'), + ), + ).toBe(true); + }); + + it('serialises concurrent invocations instead of interleaving them', async () => { + await Promise.all([restart(), restart()]); + + expect(harness.refreshes).toBe(2); + expect(phasesOf()).toEqual([ + 'dispose', + 'dispose', + 'dispose', + 'register', + 'register', + 'register', + 'dispose', + 'dispose', + 'dispose', + 'register', + 'register', + 'register', + ]); + }); +}); diff --git a/packages/vscode/src/extension.ts b/packages/vscode/src/extension.ts index 867b310..e6fddef 100644 --- a/packages/vscode/src/extension.ts +++ b/packages/vscode/src/extension.ts @@ -105,8 +105,8 @@ class ExtensionShell { ); }; - register('rstack.showMenu', () => this.#statusBar.showMenu()); register('rstack.showOutput', () => this.#channels.shell.show()); + register('rstack.restart', () => this.restart()); register('rstack.migrateSettings', () => runSettingsMigration(this.#channels.shell), ); @@ -114,6 +114,10 @@ class ExtensionShell { register(`rstack.${stack}.output.focus`, () => this.#channels.forStack(stack).show(), ); + // Owned by the shell, not the stack: a stack cannot rebuild itself, and + // the shallower alternative (bouncing just the tool's own process) leaves + // the controller's package resolution and version check stale. + register(`rstack.${stack}.restart`, () => this.restart(stack)); } } @@ -162,24 +166,122 @@ class ExtensionShell { return { ok: true }; } + /** + * The single queue every pass runs on — reconciles and restarts alike. Two + * passes must never overlap, and a caller awaiting the returned promise + * waits for its own pass only. A rejection belongs to that caller, so the + * chain keeps only the settled shape and survives it. + */ + private enqueue(task: () => Promise): Promise { + const pass = this.#reconciling.then(task); + this.#reconciling = pass.catch(() => undefined); + return pass; + } + private scheduleReconcile(): void { if (this.#disposed) { return; } - this.#reconciling = this.#reconciling.then( - () => this.reconcile(), - () => this.reconcile(), + void this.enqueue(() => this.reconcile()); + } + + /** + * `rstack.restart` (every stack) and `rstack..restart` (one) — a full + * reset, not a "retry whatever looks broken". + * + * `reconcileStack` deliberately leaves an already registered stack alone, so + * a stack that is up but wedged is never rebuilt: `node_modules` can be + * replaced or corrupted while every watched file stays untouched, and until + * now only a window reload recovered from that. This disposes the + * controllers, re-runs detection and registers every affected stack that + * still passes the gate, from scratch. + * + * It rides the shared `#reconciling` queue, so repeated or concurrent + * invocations serialise instead of racing, and the returned promise settles + * only once this restart is done. + */ + async restart(stack?: StackId): Promise { + await this.enqueue(() => this.runRestart(stack)); + } + + private async runRestart(only?: StackId): Promise { + if (this.#disposed) { + return; + } + const stacks = only ? [only] : STACK_IDS; + this.#channels.shell.info( + only ? `Restarting ${STACK_LABELS[only]}` : 'Relaunching Rstack', ); + // Per-stack isolation covers the restart too: a stack throwing on the way + // down must not keep the others from coming back up. + await Promise.allSettled( + stacks.flatMap((stack) => { + const controller = this.#controllers.get(stack); + return controller + ? [this.retire(stack, controller, { kind: 'starting' })] + : []; + }), + ); + // Teardown is slow (closing the Rslint language client, `$close()`ing the + // Rstest workers), so a `deactivate()` can land inside it. From here on + // every shell resource — the output channels above all — may already be + // disposed, and touching one throws. + if (this.#disposed) { + return; + } + try { + // A plain pass: `refresh` updates the snapshot whether or not the + // signature moved, and the reconcile below rebuilds every stack from it. + // The forced notification the lockfile path uses exists to make *live* + // controllers retry — there are none left to tell. + await this.#detection.refresh(); + } catch (error) { + if (!this.#disposed) { + this.#channels.shell.error( + `Detection failed during restart: ${errorMessage(error)}`, + ); + } + } + await this.reconcile(stacks); + if (!this.#disposed) { + this.#channels.shell.info( + only ? `${STACK_LABELS[only]} restart finished` : 'Rstack relaunched', + ); + } } - private async reconcile(): Promise { + /** + * A controller going away always means the same five things; only the state + * left on the status bar says why (`starting` for a restart about to rebuild + * it, the gate's own state when it stopped qualifying, `crashed` when it + * failed to register). Pass no state to retire it silently, which is what a + * shell already on its way out wants. + */ + private async retire( + stack: StackId, + controller: StackController, + next?: StackState, + ): Promise { + this.#controllers.delete(stack); + await this.disposeController(stack, controller); + if (!next || this.#disposed) { + return; + } + await this.setContextKey(`rstack.${stack}.active`, false); + this.#statusBar.setActive(stack, false); + this.#statusBar.setState(stack, next); + } + + private async reconcile( + stacks: readonly StackId[] = STACK_IDS, + ): Promise { if (this.#disposed) { return; } const snapshot = this.#detection.snapshot; // Per-stack isolation: one stack throwing must never affect the others. await Promise.allSettled( - STACK_IDS.map((stack) => this.reconcileStack(stack, snapshot)), + stacks.map((stack) => this.reconcileStack(stack, snapshot)), ); } @@ -191,18 +293,23 @@ class ExtensionShell { `rstack.${stack}.detected`, snapshot.isDetected(stack), ); + // Setting a context key is a round trip to the main thread, and a restart + // empties `#controllers` before getting here — so a `deactivate()` landing + // in this window would find nothing to dispose and the controller built + // below would outlive the extension. + if (this.#disposed) { + return; + } const gate = this.gate(stack, snapshot); const existing = this.#controllers.get(stack); if (!gate.ok) { if (existing) { - this.#controllers.delete(stack); - await this.disposeController(stack, existing); - await this.setContextKey(`rstack.${stack}.active`, false); - this.#statusBar.setActive(stack, false); + await this.retire(stack, existing, gate.state); + } else { + this.#statusBar.setState(stack, gate.state); } - this.#statusBar.setState(stack, gate.state); return; } @@ -223,6 +330,13 @@ class ExtensionShell { detection: snapshot, onDidChangeDetection: this.#detectionEmitter.event, }); + // `register()` is the long await here (an LSP handshake, a worker spawn), + // so `dispose()` can have run through its controller loop while this one + // was still starting. Nothing else will collect it — do it here. + if (this.#disposed) { + await this.retire(stack, controller); + return; + } if (stackExports) { this.publishStackExports(stack, stackExports); } @@ -230,19 +344,18 @@ class ExtensionShell { this.#statusBar.setActive(stack, true); this.#channels.shell.info(`${STACK_LABELS[stack]} registered`); } catch (error) { - this.#controllers.delete(stack); - await this.disposeController(stack, controller); - await this.setContextKey(`rstack.${stack}.active`, false); - this.#statusBar.setActive(stack, false); + await this.retire(stack, controller, { + kind: 'crashed', + detail: error instanceof Error ? error.message : String(error), + }); + if (this.#disposed) { + return; + } const message = errorMessage(error); this.#channels.shell.error( `${STACK_LABELS[stack]} failed to register: ${message}`, ); this.#channels.forStack(stack).error(message); - this.#statusBar.setState(stack, { - kind: 'crashed', - detail: error instanceof Error ? error.message : String(error), - }); } } @@ -254,6 +367,12 @@ class ExtensionShell { try { await controller.dispose(); } catch (error) { + // `dispose()` closes the channels after its controller loop, and a stack + // can still be shutting down then; a failed dispose must not become an + // unhandled "channel closed" on the way out. + if (this.#disposed) { + return; + } this.#channels.shell.error( `${STACK_LABELS[stack]} failed to dispose: ${errorMessage(error)}`, ); @@ -304,8 +423,7 @@ class ExtensionShell { async dispose(): Promise { this.#disposed = true; for (const [stack, controller] of [...this.#controllers]) { - this.#controllers.delete(stack); - await this.disposeController(stack, controller); + await this.retire(stack, controller); } for (const subscription of this.#subscriptions) { subscription.dispose(); diff --git a/packages/vscode/src/stacks/lint/index.ts b/packages/vscode/src/stacks/lint/index.ts index 235f028..05fe479 100644 --- a/packages/vscode/src/stacks/lint/index.ts +++ b/packages/vscode/src/stacks/lint/index.ts @@ -116,7 +116,6 @@ class RslintController implements StackController { #snapshot: DetectionSnapshot | undefined; readonly #subscriptions: vscode.Disposable[] = []; readonly #folderStates = new Map(); - #pending: Promise = Promise.resolve(); #disposed = false; async register(context: StackContext): Promise> { @@ -125,9 +124,6 @@ class RslintController implements StackController { this.#logger = new Logger(context.output); this.#subscriptions.push( - vscode.commands.registerCommand('rstack.rslint.restart', () => { - void this.restart(); - }), context.onDidChangeDetection((snapshot) => { this.#snapshot = snapshot; this.reconcileFolders({ added: [], removed: [] }); @@ -141,14 +137,17 @@ class RslintController implements StackController { }), // A `binPath`/`customBinPath` change must re-resolve the binary, which // only happens on a fresh start (upstream documents `customBinPath` as - // requiring a reload; a restart is strictly better). + // requiring a reload; a restart is strictly better). It goes through the + // shell's command rather than a local restart so the whole controller is + // rebuilt — a local one would replace the coordinator but keep this + // controller's already-resolved binary and version check. vscode.workspace.onDidChangeConfiguration((event) => { if ( event.affectsConfiguration('rstack.rslint.binPath') || event.affectsConfiguration('rstack.rslint.customBinPath') || event.affectsConfiguration('rstack.rslint.trace.server') ) { - void this.restart(); + void vscode.commands.executeCommand('rstack.rslint.restart'); } }), ); @@ -284,33 +283,6 @@ class RslintController implements StackController { ); } - /** - * Serializes restart/dispose. Two restarts racing (a settings change plus the - * palette command) would otherwise interleave close and start and leak a - * coordinator that nothing holds a reference to any more. - */ - private enqueue(task: () => Promise): Promise { - this.#pending = this.#pending.then(task, task); - return this.#pending; - } - - /** - * `rstack.rslint.restart`. The coordinator is single-use once closed, so a - * restart replaces it (and the document router) wholesale — the same shape - * upstream's commented-out `rslint.restart` would have needed. - */ - private async restart(): Promise { - await this.enqueue(async () => { - if (this.#disposed || !this.#context) { - return; - } - this.#logger?.info('Restarting the Rslint language server'); - await this.closeCoordinator(); - this.#folderStates.clear(); - this.startCoordinator(); - }); - } - private async closeCoordinator(): Promise { const coordinator = this.#coordinator; this.#coordinator = undefined; @@ -329,15 +301,11 @@ class RslintController implements StackController { for (const subscription of this.#subscriptions.splice(0)) { subscription.dispose(); } - // Behind the same queue as `restart`, so an in-flight restart finishes - // (or no-ops on `#disposed`) before the language servers are torn down. - await this.enqueue(async () => { - await this.closeCoordinator(); - this.#folderStates.clear(); - this.#logger = undefined; - this.#context = undefined; - this.#snapshot = undefined; - }); + await this.closeCoordinator(); + this.#folderStates.clear(); + this.#logger = undefined; + this.#context = undefined; + this.#snapshot = undefined; } } diff --git a/packages/vscode/src/statusBar.ts b/packages/vscode/src/statusBar.ts index 2e48559..8f57470 100644 --- a/packages/vscode/src/statusBar.ts +++ b/packages/vscode/src/statusBar.ts @@ -14,17 +14,36 @@ const OUTPUT_COMMANDS: Readonly> = { fmt: 'rstack.fmt.output.focus', }; -const RESTART_COMMANDS: Partial>> = { +const RESTART_COMMANDS: Readonly> = { rslint: 'rstack.rslint.restart', + rstest: 'rstack.rstest.restart', + fmt: 'rstack.fmt.restart', }; -const STATE_ICONS: Readonly> = { - 'not-detected': '$(circle-slash)', - disabled: '$(circle-slash)', - starting: '$(loading~spin)', - running: '$(check)', - crashed: '$(error)', - 'version-mismatch': '$(warning)', +/** + * Icon and hover colour per state. `color` is a theme colour id with `.` + * replaced by `-`, the form VS Code exposes as a CSS variable; the markdown + * sanitizer accepts `var(--vscode-*)` on a `` and nothing else, so the + * hover picks up the user's theme instead of hard-coded hexes. + */ +const STATE_STYLES: Readonly< + Record +> = { + // The two off-states share a glyph but not a colour: nothing was found for + // this workspace (the weakest thing on the row — `disabledForeground` is the + // colour VS Code reserves for "not available") versus somebody turned it off + // on purpose, which is worth actually reading. Glyphs from other icon sets + // (the debug breakpoints, say) are drawn at their own optical size and stand + // out of a row of plain codicons — keep every state on one set. + 'not-detected': { icon: '$(circle-slash)', color: 'disabledForeground' }, + disabled: { icon: '$(circle-slash)', color: 'descriptionForeground' }, + starting: { icon: '$(loading~spin)', color: 'descriptionForeground' }, + running: { icon: '$(check)', color: 'testing-iconPassed' }, + crashed: { icon: '$(error)', color: 'testing-iconFailed' }, + 'version-mismatch': { + icon: '$(warning)', + color: 'editorWarning-foreground', + }, }; const stateText = (state: StackState): string => { @@ -44,10 +63,37 @@ const stateText = (state: StackState): string => { } }; +/** An icon-only command link, with the wording moved to its native tooltip. */ +const action = (command: string, title: string, icon: string): string => + `${icon}`; + +/** + * A labelled command as a table row, so its icon lands in the same column as + * the stack rows'. Icon and label are separate cells and therefore separate + * links to the same command — one `` cannot span two cells. + */ +const link = (command: string, icon: string, label: string): string => { + const href = ``; + return `${href}${icon}${href} ${label}`; +}; + +const escapeAttribute = (value: string): string => + value + .replace(/&/g, '&') + .replace(//g, '>') + .replace(/"/g, '"'); + /** * The single always-present status bar item. It is visible * whenever the extension is installed and enabled, even when nothing is * detected — it is the answer to "is the extension broken or just idle?". + * + * The hover is the only surface: it carries the per-stack state and every + * action, and clicking the item goes straight to the extension log. There is + * deliberately no QuickPick behind the item — a menu that repeats what the + * hover already shows is two renderings of one model, and every peer + * (Biome, oxc, Prettier) ships exactly one of the two. */ export class StatusBar implements vscode.Disposable { readonly #item: vscode.StatusBarItem; @@ -68,7 +114,10 @@ export class StatusBar implements vscode.Disposable { 100, ); this.#item.name = 'Rstack'; - this.#item.command = 'rstack.showMenu'; + // Clicking goes to the extension log, the way Prettier's item does. It is + // the one action that is useful in every state, including the states where + // no stack is active and there is nothing else to offer. + this.#item.command = 'rstack.showOutput'; this.render(); this.#item.show(); } @@ -105,48 +154,7 @@ export class StatusBar implements vscode.Disposable { } #canRestart(stack: StackId): boolean { - return RESTART_COMMANDS[stack] !== undefined && this.#active.has(stack); - } - - /** The QuickPick behind the status bar item. */ - async showMenu(): Promise { - type Item = vscode.QuickPickItem & { readonly command?: string }; - const items: Item[] = []; - for (const stack of STACK_IDS) { - const state = this.stateOf(stack); - items.push({ - label: `${STATE_ICONS[state.kind]} ${STACK_LABELS[stack]}`, - description: stateText(state), - detail: 'Show output', - command: OUTPUT_COMMANDS[stack], - }); - const restart = RESTART_COMMANDS[stack]; - if (restart && this.#canRestart(stack)) { - items.push({ - label: `$(refresh) Restart ${STACK_LABELS[stack]}`, - command: restart, - }); - } - } - items.push( - { label: '', kind: vscode.QuickPickItemKind.Separator }, - { - label: '$(output) Show Rstack extension log', - command: 'rstack.showOutput', - }, - { - label: '$(arrow-right) Migrate Rslint/Rstest settings', - command: 'rstack.migrateSettings', - }, - ); - - const picked = await vscode.window.showQuickPick(items, { - title: 'Rstack', - placeHolder: 'Select an action', - }); - if (picked?.command) { - await vscode.commands.executeCommand(picked.command); - } + return this.#active.has(stack); } private render(): void { @@ -179,7 +187,7 @@ export class StatusBar implements vscode.Disposable { this.#item.backgroundColor = undefined; break; default: - this.#item.text = '$(layers) Rstack'; + this.#item.text = '$(zap) Rstack'; this.#item.backgroundColor = undefined; break; } @@ -187,20 +195,86 @@ export class StatusBar implements vscode.Disposable { const tooltip = new vscode.MarkdownString(undefined, true); // Command links are only rendered in trusted markdown. tooltip.isTrusted = true; - tooltip.appendMarkdown('**Rstack**\n\n'); - for (const stack of STACK_IDS) { + // The stack rows are a raw `` so the three columns line up; markdown + // has no alignment short of a table with a visible header row, and one row + // per paragraph left the action icons ragged. Raw html suppresses markdown + // inside it, so the cells use ``/`` rather than `**`/`[]()` — the + // sanitizer keeps `command:` hrefs as long as the string stays trusted. + tooltip.supportHtml = true; + const rows = STACK_IDS.map((stack) => { const state = this.stateOf(stack); - const links = [`[Output](command:${OUTPUT_COMMANDS[stack]})`]; - const restart = RESTART_COMMANDS[stack]; - if (restart && this.#canRestart(stack)) { - links.push(`[Restart](command:${restart})`); + const style = STATE_STYLES[state.kind]; + const label = STACK_LABELS[stack]; + // The per-stack actions repeat once per row, so they are icon-only: the + // row already names the stack, and the link title carries the wording for + // anyone who hovers the icon. The global row below stays text — it + // appears once and has no row label to lean on. + const actions = [ + action(OUTPUT_COMMANDS[stack], `Show the ${label} log`, '$(selection)'), + ]; + if (this.#canRestart(stack)) { + // Titled apart from "Relaunch extension" below: this one rebuilds only + // this stack and leaves the others running. + actions.push( + action(RESTART_COMMANDS[stack], `Restart ${label}`, '$(refresh)'), + ); } - tooltip.appendMarkdown( - `${STATE_ICONS[state.kind]} **${STACK_LABELS[stack]}** — ${stateText( - state, - )} · ${links.join(' · ')}\n\n`, + // The state text is the icon's title rather than row text: spelling out + // "running — 2 folders" on every row is mostly noise once the icon says + // it, and the details worth reading (a crash message, a version + // mismatch) are exactly the long ones. `state.detail` is arbitrary text a + // stack produced, hence the escaping. + // Icon size is not adjustable here: the hover renders codicons at + // `font-size: inherit` and the sanitizer drops `font-size` from a span's + // style, leaving heading tags as the only lever — and those carry the + // hover's `h1-h6 { margin: 8px 0 }`, which pads out every row. Not worth + // it; the icons stay at the row's own size. + const status = + `${style.icon}`; + // The table stays sized to its content. Stretching it with + // `width="100%"` does push the actions to the card's edge, but the card + // is as wide as the widest line below, so the row ends up mostly gap and + // the action cell gets squeezed until its two icons wrap onto separate + // lines. Right-aligning inside the natural column is as far as this goes. + return ( + `` + + `` ); - } + }); + // Same table as the stacks, so all six icons share one column and one gap + // to their label. A second table (or a markdown paragraph) would size its + // columns independently and the two halves would drift apart. + // + // One row per action rather than three across: the hover has no width of + // its own, it sizes to its content, so three labelled actions on one line + // set the card's width and leave the rows above swimming in it. + // + // Unlike the per-stack restarts, "Relaunch" is unconditional: it is the + // action for "nothing is active", which is precisely when no per-stack + // restart is offered. + const global = [ + link('rstack.restart', '$(debug-restart)', 'Relaunch'), + link('rstack.showOutput', '$(selection)', 'Extension log'), + link('rstack.migrateSettings', '$(arrow-right)', 'Migrate settings'), + ]; + tooltip.appendMarkdown( + `
${status} ${label}  ${actions.join(' ')}
${rows.join('')}` + + // The gap under the divider is an *empty* spacer row with an explicit + // `height` — the one pixel-precise spacing lever in sanitized html. + // Everything line-based was tried and is quantized to a full row: cell + // padding is unreachable (the sanitizer keeps `style` only on a span, + // colours only), a `
` costs a line-height (too much), no spacer + // leaves the hover's `hr { margin-bottom: -4px }` hugging the next row + // (too little), and shrinking a blank line with `` does nothing + // because the cell's own strut keeps the line box at the td's + // font-height. An empty cell has no line box at all, so its `height` + // attribute (allowlisted) is what it says. Above the rule the hr's own + // 4px top margin is enough. + '' + + '' + + `${global.join('')}
`, + ); this.#item.tooltip = tooltip; } diff --git a/packages/vscode/tests/e2e/suite/shell.test.ts b/packages/vscode/tests/e2e/suite/shell.test.ts index 8431366..b7d64e7 100644 --- a/packages/vscode/tests/e2e/suite/shell.test.ts +++ b/packages/vscode/tests/e2e/suite/shell.test.ts @@ -1,6 +1,7 @@ import assert from 'node:assert/strict'; import * as vscode from 'vscode'; -import { delay, eventually } from './helpers'; +import type { RstackExtensionExports } from '../../../src/types'; +import { eventually } from './helpers'; const EXTENSION_ID = 'rstack.rstack'; @@ -31,39 +32,53 @@ suite('shell', () => { test('registers the shell commands', async () => { const commands = await vscode.commands.getCommands(true); for (const command of [ - 'rstack.showMenu', 'rstack.showOutput', + 'rstack.restart', 'rstack.migrateSettings', 'rstack.rslint.output.focus', + 'rstack.rslint.restart', 'rstack.rstest.output.focus', + 'rstack.rstest.restart', 'rstack.fmt.output.focus', + 'rstack.fmt.restart', ]) { assert.ok(commands.includes(command), `missing command ${command}`); } }); - test('has a status bar item whose menu opens', async () => { + test('has a status bar item whose click target works', async () => { // VS Code exposes no API to enumerate another extension's status bar items, // so the item itself cannot be asserted on directly. What *is* observable - // is its command: the item is created with `command = 'rstack.showMenu'` - // and shown unconditionally, so a `showMenu` that opens a QuickPick without - // throwing is the strongest available evidence that the always-present - // status bar item exists and is wired up. - let failure: unknown; - const menu = Promise.resolve( - vscode.commands.executeCommand('rstack.showMenu'), - ).catch((error: unknown) => { - failure = error; - }); + // is its command: the item is created with `command = 'rstack.showOutput'` + // and shown unconditionally, so a `showOutput` that reveals the channel + // without throwing is the strongest available evidence that the + // always-present status bar item exists and is wired up. + await vscode.commands.executeCommand('rstack.showOutput'); + }); + + test('rebuilds the live stacks on rstack.restart', async () => { + // The restart is a full reset: every controller is disposed and rebuilt, + // so the exports a stack publishes at registration must be a *different* + // object afterwards. That is the only externally visible proof that the + // stack was rebuilt rather than left alone (`reconcileStack` returns early + // for a stack that is already registered). + const extension = + vscode.extensions.getExtension(EXTENSION_ID); + assert.ok(extension, `${EXTENSION_ID} is not installed in the test host`); + const api = await extension.activate(); + + const before = await api.whenStackActive('rstest'); - await delay(1_000); - await vscode.commands.executeCommand('workbench.action.closeQuickOpen'); - await Promise.race([menu, delay(5_000)]); + // The command resolves only once the whole restart is done, so no polling + // is needed to observe the result. + await vscode.commands.executeCommand('rstack.restart'); - assert.equal( - failure, - undefined, - `rstack.showMenu failed: ${String(failure)}`, + const after = api.getStackExports('rstest'); + assert.ok(after, 'the Rstest stack did not come back after the restart'); + assert.notEqual( + after, + before, + 'the restart must rebuild the controller, not keep the old one', ); }); From 0779bf26acbc364fc5fe236acf9baf1c091f7a42 Mon Sep 17 00:00:00 2001 From: fi3ework Date: Wed, 5 Aug 2026 22:17:57 +0800 Subject: [PATCH 2/7] refactor: derive stack command ids and give stacks a restart seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three follow-ups from a cleanup pass over the restart work: - `stackCommand(stack, verb)` in types.ts is now the single place a per-stack command id is spelled. The shell registers through it and the status bar links through it, so the two OUTPUT_COMMANDS/RESTART_COMMANDS tables are gone — they had become hand-copied templates whose drift from the registration site would not be a type error. - `StackContext.requestRestart(reason)` replaces the Rslint stack reaching back through the command registry. Restart was already a shell service; it is now injected like every other one, and the reason reaches the log so a settings-triggered restart says what moved. - `dispose()` tears controllers down in parallel, matching the restart path, and drops `clearProjectModuleCache` which had no callers. Also folds the status bar's two anchor builders into one so escaping is a property of the markup rather than a per-call-site obligation, inlines `#canRestart`, and trims the rejected-alternative journal from render(). --- packages/vscode/src/extension.test.ts | 35 +--- packages/vscode/src/extension.ts | 58 ++++--- packages/vscode/src/stacks/lint/index.ts | 19 +-- .../vscode/src/stacks/lint/projectModules.ts | 5 - packages/vscode/src/statusBar.ts | 161 ++++++++---------- packages/vscode/src/types.ts | 22 +++ 6 files changed, 140 insertions(+), 160 deletions(-) diff --git a/packages/vscode/src/extension.test.ts b/packages/vscode/src/extension.test.ts index b8ac7e9..ef7f4cf 100644 --- a/packages/vscode/src/extension.test.ts +++ b/packages/vscode/src/extension.test.ts @@ -1,6 +1,3 @@ -import { afterEach, beforeEach, describe, expect, it, rs } from '@rstest/core'; -import type vscode from 'vscode'; - /** * The shell itself is the unit under test: `activate()` runs for real and the * commands it contributes are invoked through the recorded command registry, @@ -9,6 +6,9 @@ import type vscode from 'vscode'; * tests run in plain Node with no extension host (unit tests are Rstest, E2E is * Electron, and E2E stays the ground truth for editor behaviour). */ +import { afterEach, beforeEach, describe, expect, it, rs } from '@rstest/core'; +import type vscode from 'vscode'; + interface FakeController { register(): Promise>; dispose(): Promise; @@ -51,26 +51,14 @@ const harness = rs.hoisted(() => { }); rs.mock('vscode', () => { + const disposable = { dispose: () => undefined }; + // Detection is stubbed out below, so nothing here ever fires the shell's + // emitter — only its construction and disposal are reached. class EventEmitter { - readonly #listeners = new Set<(value: unknown) => void>(); - readonly event = (listener: (value: unknown) => void) => { - this.#listeners.add(listener); - return { - dispose: () => { - this.#listeners.delete(listener); - }, - }; - }; - fire(value: unknown): void { - for (const listener of [...this.#listeners]) { - listener(value); - } - } - dispose(): void { - this.#listeners.clear(); - } + readonly event = () => disposable; + fire(): void {} + dispose(): void {} } - const disposable = { dispose: () => undefined }; const createOutputChannel = (name: string) => { // Only the shell channel is recorded — the failure reports the user is // supposed to find live there. @@ -311,11 +299,6 @@ describe('the shell restart command', () => { expect(harness.contextKeys.get('rstack.rslint.active')).toBe(true); }); - it('re-runs detection before rebuilding', async () => { - await restart(); - expect(harness.refreshes).toBe(1); - }); - it('rebuilds a stack whose detection did not move at all', async () => { // The whole point of the command: nothing observable changed, yet every // controller is replaced, because `node_modules` may have been. diff --git a/packages/vscode/src/extension.ts b/packages/vscode/src/extension.ts index e6fddef..0ad3109 100644 --- a/packages/vscode/src/extension.ts +++ b/packages/vscode/src/extension.ts @@ -12,6 +12,7 @@ import { type StackState, STACK_IDS, STACK_LABELS, + stackCommand, } from './types'; import { createFmtController } from './stacks/fmt'; import { createRslintController } from './stacks/lint'; @@ -111,13 +112,14 @@ class ExtensionShell { runSettingsMigration(this.#channels.shell), ); for (const stack of STACK_IDS) { - register(`rstack.${stack}.output.focus`, () => + register(stackCommand(stack, 'output.focus'), () => this.#channels.forStack(stack).show(), ); // Owned by the shell, not the stack: a stack cannot rebuild itself, and // the shallower alternative (bouncing just the tool's own process) leaves - // the controller's package resolution and version check stale. - register(`rstack.${stack}.restart`, () => this.restart(stack)); + // the controller's package resolution and version check stale. Stacks + // reach the same operation through `StackContext.requestRestart`. + register(stackCommand(stack, 'restart'), () => this.restart(stack)); } } @@ -196,31 +198,30 @@ class ExtensionShell { * controllers, re-runs detection and registers every affected stack that * still passes the gate, from scratch. * - * It rides the shared `#reconciling` queue, so repeated or concurrent - * invocations serialise instead of racing, and the returned promise settles - * only once this restart is done. + * `reason` is for the callers that are not a user picking the command — + * `StackContext.requestRestart` passes what moved. */ - async restart(stack?: StackId): Promise { - await this.enqueue(() => this.runRestart(stack)); + restart(stack?: StackId, reason?: string): Promise { + return this.enqueue(() => this.runRestart(stack, reason)); } - private async runRestart(only?: StackId): Promise { + private async runRestart(only?: StackId, reason?: string): Promise { if (this.#disposed) { return; } const stacks = only ? [only] : STACK_IDS; + const what = only ? STACK_LABELS[only] : 'Rstack'; this.#channels.shell.info( - only ? `Restarting ${STACK_LABELS[only]}` : 'Relaunching Rstack', + `Restarting ${what}${reason ? ` (${reason})` : ''}`, ); // Per-stack isolation covers the restart too: a stack throwing on the way // down must not keep the others from coming back up. await Promise.allSettled( - stacks.flatMap((stack) => { - const controller = this.#controllers.get(stack); - return controller - ? [this.retire(stack, controller, { kind: 'starting' })] - : []; - }), + [...this.#controllers] + .filter(([stack]) => stacks.includes(stack)) + .map(([stack, controller]) => + this.retire(stack, controller, { kind: 'starting' }), + ), ); // Teardown is slow (closing the Rslint language client, `$close()`ing the // Rstest workers), so a `deactivate()` can land inside it. From here on @@ -244,18 +245,15 @@ class ExtensionShell { } await this.reconcile(stacks); if (!this.#disposed) { - this.#channels.shell.info( - only ? `${STACK_LABELS[only]} restart finished` : 'Rstack relaunched', - ); + this.#channels.shell.info(`${what} restart finished`); } } /** - * A controller going away always means the same five things; only the state - * left on the status bar says why (`starting` for a restart about to rebuild - * it, the gate's own state when it stopped qualifying, `crashed` when it - * failed to register). Pass no state to retire it silently, which is what a - * shell already on its way out wants. + * Retires a controller. Only the state left on the status bar says why + * (`starting` for a restart about to rebuild it, the gate's own state when it + * stopped qualifying, `crashed` when it failed to register). Pass no state to + * retire it silently, which is what a shell already on its way out wants. */ private async retire( stack: StackId, @@ -329,6 +327,7 @@ class ExtensionShell { status: this.#statusBar.reporterFor(stack), detection: snapshot, onDidChangeDetection: this.#detectionEmitter.event, + requestRestart: (reason) => this.restart(stack, reason), }); // `register()` is the long await here (an LSP handshake, a worker spawn), // so `dispose()` can have run through its controller loop while this one @@ -422,9 +421,14 @@ class ExtensionShell { async dispose(): Promise { this.#disposed = true; - for (const [stack, controller] of [...this.#controllers]) { - await this.retire(stack, controller); - } + // Same shape as the restart teardown: per-stack isolation, and deactivate + // runs against VS Code's shutdown budget, so the slow ones (the Rslint + // client's graceful-then-forced kill) overlap instead of queueing. + await Promise.allSettled( + [...this.#controllers].map(([stack, controller]) => + this.retire(stack, controller), + ), + ); for (const subscription of this.#subscriptions) { subscription.dispose(); } diff --git a/packages/vscode/src/stacks/lint/index.ts b/packages/vscode/src/stacks/lint/index.ts index 05fe479..c4c0599 100644 --- a/packages/vscode/src/stacks/lint/index.ts +++ b/packages/vscode/src/stacks/lint/index.ts @@ -137,17 +137,16 @@ class RslintController implements StackController { }), // A `binPath`/`customBinPath` change must re-resolve the binary, which // only happens on a fresh start (upstream documents `customBinPath` as - // requiring a reload; a restart is strictly better). It goes through the - // shell's command rather than a local restart so the whole controller is - // rebuilt — a local one would replace the coordinator but keep this - // controller's already-resolved binary and version check. + // requiring a reload; a restart is strictly better). It asks the shell + // for a full rebuild rather than restarting locally — a local restart + // would replace the coordinator but keep this controller's + // already-resolved binary and version check. vscode.workspace.onDidChangeConfiguration((event) => { - if ( - event.affectsConfiguration('rstack.rslint.binPath') || - event.affectsConfiguration('rstack.rslint.customBinPath') || - event.affectsConfiguration('rstack.rslint.trace.server') - ) { - void vscode.commands.executeCommand('rstack.rslint.restart'); + for (const setting of ['binPath', 'customBinPath', 'trace.server']) { + if (event.affectsConfiguration(`rstack.rslint.${setting}`)) { + void context.requestRestart(`rstack.rslint.${setting} changed`); + return; + } } }), ); diff --git a/packages/vscode/src/stacks/lint/projectModules.ts b/packages/vscode/src/stacks/lint/projectModules.ts index 73953fd..3bb517b 100644 --- a/packages/vscode/src/stacks/lint/projectModules.ts +++ b/packages/vscode/src/stacks/lint/projectModules.ts @@ -35,8 +35,3 @@ export const importProjectModule = async ( } return pending; }; - -/** Test seam / teardown helper: drops the memoized module promises. */ -export const clearProjectModuleCache = (): void => { - cache.clear(); -}; diff --git a/packages/vscode/src/statusBar.ts b/packages/vscode/src/statusBar.ts index 8f57470..b629c91 100644 --- a/packages/vscode/src/statusBar.ts +++ b/packages/vscode/src/statusBar.ts @@ -5,21 +5,9 @@ import { type StatusReporter, STACK_IDS, STACK_LABELS, + stackCommand, } from './types'; -/** Commands the status bar hover links to, per stack. */ -const OUTPUT_COMMANDS: Readonly> = { - rslint: 'rstack.rslint.output.focus', - rstest: 'rstack.rstest.output.focus', - fmt: 'rstack.fmt.output.focus', -}; - -const RESTART_COMMANDS: Readonly> = { - rslint: 'rstack.rslint.restart', - rstest: 'rstack.rstest.restart', - fmt: 'rstack.fmt.restart', -}; - /** * Icon and hover colour per state. `color` is a theme colour id with `.` * replaced by `-`, the form VS Code exposes as a CSS variable; the markdown @@ -29,12 +17,10 @@ const RESTART_COMMANDS: Readonly> = { const STATE_STYLES: Readonly< Record > = { - // The two off-states share a glyph but not a colour: nothing was found for - // this workspace (the weakest thing on the row — `disabledForeground` is the - // colour VS Code reserves for "not available") versus somebody turned it off - // on purpose, which is worth actually reading. Glyphs from other icon sets - // (the debug breakpoints, say) are drawn at their own optical size and stand - // out of a row of plain codicons — keep every state on one set. + // The two off-states share a glyph but not a colour: nothing found here (the + // weakest thing on the row) versus somebody turned it off on purpose, which + // is worth reading. Keep every state on the plain codicon set — glyphs from + // the debug sets are drawn at their own optical size and stick out. 'not-detected': { icon: '$(circle-slash)', color: 'disabledForeground' }, disabled: { icon: '$(circle-slash)', color: 'descriptionForeground' }, starting: { icon: '$(loading~spin)', color: 'descriptionForeground' }, @@ -63,20 +49,6 @@ const stateText = (state: StackState): string => { } }; -/** An icon-only command link, with the wording moved to its native tooltip. */ -const action = (command: string, title: string, icon: string): string => - `${icon}`; - -/** - * A labelled command as a table row, so its icon lands in the same column as - * the stack rows'. Icon and label are separate cells and therefore separate - * links to the same command — one `` cannot span two cells. - */ -const link = (command: string, icon: string, label: string): string => { - const href = ``; - return `${href}${icon}${href} ${label}`; -}; - const escapeAttribute = (value: string): string => value .replace(/&/g, '&') @@ -84,6 +56,25 @@ const escapeAttribute = (value: string): string => .replace(/>/g, '>') .replace(/"/g, '"'); +/** + * The one anchor builder, so escaping is a property of the markup rather than + * something each call site has to remember. `title` becomes the icon's native + * tooltip, which is where the wording goes for the icon-only actions. + */ +const anchor = (command: string, body: string, title?: string): string => + `${body}`; + +/** + * A labelled command as a table row, so its icon lands in the same column as + * the stack rows'. Icon and label are separate cells and therefore separate + * anchors to the same command — one `` cannot span two cells. + */ +const actionRow = (command: string, icon: string, label: string): string => + `${anchor(command, icon)}` + + `${anchor(command, ` ${label}`)}`; + /** * The single always-present status bar item. It is visible * whenever the extension is installed and enabled, even when nothing is @@ -100,11 +91,10 @@ export class StatusBar implements vscode.Disposable { readonly #states = new Map( STACK_IDS.map((stack) => [stack, { kind: 'not-detected' }]), ); - // Stacks whose controller is currently registered. Restart availability - // tracks this, not the state kind: a state cannot distinguish "crashed - // while running" (controller alive, its restart command exists) from - // "failed to register" (controller disposed, the command with it), and a - // disabled stack never registered the command at all. + // Stacks whose controller is currently registered. The restart action tracks + // this rather than the state kind, which cannot tell "crashed while running" + // (controller alive, worth rebuilding) from "failed to register" (already + // disposed, and the reconcile that follows will retry it anyway). readonly #active = new Set(); constructor() { @@ -153,10 +143,6 @@ export class StatusBar implements vscode.Disposable { this.render(); } - #canRestart(stack: StackId): boolean { - return this.#active.has(stack); - } - private render(): void { const states = STACK_IDS.map((stack) => this.stateOf(stack)); const worst = states.find((state) => state.kind === 'crashed') @@ -206,75 +192,66 @@ export class StatusBar implements vscode.Disposable { const style = STATE_STYLES[state.kind]; const label = STACK_LABELS[stack]; // The per-stack actions repeat once per row, so they are icon-only: the - // row already names the stack, and the link title carries the wording for - // anyone who hovers the icon. The global row below stays text — it - // appears once and has no row label to lean on. + // row already names the stack and the anchor title carries the wording. const actions = [ - action(OUTPUT_COMMANDS[stack], `Show the ${label} log`, '$(selection)'), + anchor( + stackCommand(stack, 'output.focus'), + '$(selection)', + `Show the ${label} log`, + ), ]; - if (this.#canRestart(stack)) { - // Titled apart from "Relaunch extension" below: this one rebuilds only - // this stack and leaves the others running. + if (this.#active.has(stack)) { + // Titled apart from "Relaunch" below: this one rebuilds only this + // stack and leaves the others running. actions.push( - action(RESTART_COMMANDS[stack], `Restart ${label}`, '$(refresh)'), + anchor( + stackCommand(stack, 'restart'), + '$(refresh)', + `Restart ${label}`, + ), ); } // The state text is the icon's title rather than row text: spelling out - // "running — 2 folders" on every row is mostly noise once the icon says - // it, and the details worth reading (a crash message, a version - // mismatch) are exactly the long ones. `state.detail` is arbitrary text a - // stack produced, hence the escaping. - // Icon size is not adjustable here: the hover renders codicons at - // `font-size: inherit` and the sanitizer drops `font-size` from a span's - // style, leaving heading tags as the only lever — and those carry the - // hover's `h1-h6 { margin: 8px 0 }`, which pads out every row. Not worth - // it; the icons stay at the row's own size. + // "running — 2 folders" on every row is noise once the icon says it, and + // the details worth reading (a crash message, a version mismatch) are + // exactly the long ones. `state.detail` is arbitrary text a stack + // produced, hence the escaping. const status = `${style.icon}`; - // The table stays sized to its content. Stretching it with - // `width="100%"` does push the actions to the card's edge, but the card - // is as wide as the widest line below, so the row ends up mostly gap and - // the action cell gets squeezed until its two icons wrap onto separate - // lines. Right-aligning inside the natural column is as far as this goes. return ( `${status} ${label}  ` + `${actions.join(' ')}` ); }); - // Same table as the stacks, so all six icons share one column and one gap - // to their label. A second table (or a markdown paragraph) would size its - // columns independently and the two halves would drift apart. - // - // One row per action rather than three across: the hover has no width of - // its own, it sizes to its content, so three labelled actions on one line - // set the card's width and leave the rows above swimming in it. + // In the same table as the stacks so all six icons share one column; a + // second table would size its columns independently and the two halves + // would drift apart. One row per action rather than three across, for the + // same reason the actions above are icon-only — the hover sizes to its + // content, so the widest line sets the card's width. // // Unlike the per-stack restarts, "Relaunch" is unconditional: it is the // action for "nothing is active", which is precisely when no per-stack // restart is offered. - const global = [ - link('rstack.restart', '$(debug-restart)', 'Relaunch'), - link('rstack.showOutput', '$(selection)', 'Extension log'), - link('rstack.migrateSettings', '$(arrow-right)', 'Migrate settings'), + const shellActions = [ + actionRow('rstack.restart', '$(debug-restart)', 'Relaunch'), + actionRow('rstack.showOutput', '$(selection)', 'Extension log'), + actionRow('rstack.migrateSettings', '$(arrow-right)', 'Migrate settings'), ]; - tooltip.appendMarkdown( - `${rows.join('')}` + - // The gap under the divider is an *empty* spacer row with an explicit - // `height` — the one pixel-precise spacing lever in sanitized html. - // Everything line-based was tried and is quantized to a full row: cell - // padding is unreachable (the sanitizer keeps `style` only on a span, - // colours only), a `
` costs a line-height (too much), no spacer - // leaves the hover's `hr { margin-bottom: -4px }` hugging the next row - // (too little), and shrinking a blank line with `` does nothing - // because the cell's own strut keeps the line box at the td's - // font-height. An empty cell has no line box at all, so its `height` - // attribute (allowlisted) is what it says. Above the rule the hr's own - // 4px top margin is enough. - '' + - '' + - `${global.join('')}
`, - ); + // The gap under the divider is an empty spacer row with an explicit + // `height`, the one pixel-precise spacing lever sanitized html has left: + // cell padding is unreachable (`style` survives only on a span, colours + // only) and everything line-based is quantized to a whole row. An empty + // cell has no line box, so its `height` is what it says. Above the rule the + // hover's own `hr { margin-top: 4px }` is enough — and its + // `margin-bottom: -4px` is why the underside needs the spacer at all. + const body = [ + ...rows, + '
', + '', + ...shellActions, + ].join(''); + tooltip.appendMarkdown(`${body}
`); this.#item.tooltip = tooltip; } diff --git a/packages/vscode/src/types.ts b/packages/vscode/src/types.ts index 5f31eb6..96851a3 100644 --- a/packages/vscode/src/types.ts +++ b/packages/vscode/src/types.ts @@ -15,6 +15,17 @@ export const STACK_LABELS: Readonly> = { fmt: 'rs fmt', }; +/** + * The one place a per-stack command id is spelled. Both ends — the shell that + * registers these and the status bar that links to them — go through here, so + * a renamed verb cannot leave one side pointing at a command that no longer + * exists (a dead hover link is not a type error). + */ +export const stackCommand = ( + stack: StackId, + verb: 'restart' | 'output.focus', +): string => `rstack.${stack}.${verb}`; + /** * Per-stack state machine surfaced by the status bar hover. * @@ -94,6 +105,17 @@ export interface StackContext { * only has to reconcile its own per-folder runtimes. */ readonly onDidChangeDetection: vscode.Event; + /** + * Asks the shell to rebuild this stack from scratch — the same thing + * `rstack..restart` does. A stack cannot rebuild itself (its own + * controller is what gets replaced), and settling for a shallower local + * restart would keep the controller's already-resolved binary and version + * check, which is the staleness the rebuild exists to clear. + * + * `reason` goes to the shell log, so a restart nobody asked for out loud + * still says where it came from. + */ + readonly requestRestart: (reason: string) => Promise; } /** From f20df296727120ec59e76f4d4df74e68adf02917 Mon Sep 17 00:00:00 2001 From: fi3ework Date: Thu, 6 Aug 2026 12:46:38 +0800 Subject: [PATCH 3/7] docs: state what a restart cannot recover MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A restart re-resolves binaries and package versions and respawns every tool process, but the config-loader and eslint-plugin modules imported from the project stay in Node's ESM registry for the lifetime of the window. Verified: clearing the local memo in projectModules.ts hands back the identical module object, and a cache-busting query reloads only the entry module — relative specifiers inside it do not inherit the query, so the result is a fresh entry over stale dependencies. Records the limit in the README and the reasoning in AGENTS.md so nobody adds an invalidation hook that cannot work. --- packages/vscode/AGENTS.md | 1 + packages/vscode/README.md | 4 +++- packages/vscode/src/stacks/lint/projectModules.ts | 12 ++++++++++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/packages/vscode/AGENTS.md b/packages/vscode/AGENTS.md index c1abf10..dc72c6e 100644 --- a/packages/vscode/AGENTS.md +++ b/packages/vscode/AGENTS.md @@ -32,6 +32,7 @@ One extension replacing the standalone `rstack.rslint` and `rstack.rstest` exten - The lint × `rstack.config.*` bridge was built and deliberately removed: a partial editor-side bridge gave wrong results, and a correct one needs upstream work first. `TODO(rstack-bridge)` markers carry the plan. Do not reintroduce a partial bridge. - The test × `rstack.config.*` bridge stays thin on purpose: it points the upstream machinery at rstack's shipped shim and lets the shim interpret the config inside the worker, same as the CLI. Never re-implement rstack config semantics in the extension. - The fmt stack is a stub on purpose. The MVP will spawn `rs fmt --stdin-filepath` with cwd = the config directory (forced by rs fmt's cwd-only config resolution); the endgame is an upstream LSP, so do not add a warm-process middle tier or "fix" the stub into an error state. +- `projectModules.ts` has no cache-invalidation hook and restart must not grow one. Node's ESM registry is keyed by resolved URL and process-lifetime, so clearing the local memo hands back the identical module object (verified); a `?epoch=` query does reload the entry but relative specifiers inside it do not inherit the query, yielding a fresh entry over stale dependencies. In-place reinstalls under an unchanged path need a window reload — say so, don't fake it. - The VSIX is platform-targeted for exactly one reason: the test stack's AST collection loads a native parser binding. Do not add another native dependency — it multiplies the release matrix. ## Testing diff --git a/packages/vscode/README.md b/packages/vscode/README.md index c0499ca..300c25a 100644 --- a/packages/vscode/README.md +++ b/packages/vscode/README.md @@ -26,7 +26,9 @@ The extension activates on startup, then decides **per workspace folder** which | Rstest | `rstest.config.{mjs,ts,js,cjs,mts,cts}` (configurable) or `rstack.config.*` | | rstack-cli | `rstack.config.*` or `node_modules/.bin/rs` | -Config files and lockfiles are watched, so detection re-runs without a window reload. When something changes that none of those files record — a reinstall that leaves the lockfile untouched, or a `node_modules` that ends up broken — run **Rstack: Relaunch Extension** from the Command Palette (also on the status bar hover) to tear every tool down and start over. To rebuild a single tool, use **Rstack: Restart Rslint** / **Restart Rstest** / **Restart rs fmt**. Deprecated `rslint.json` / `rslint.jsonc` configs are **not** detection signals — migrate them with `rslint --init`. +Config files and lockfiles are watched, so detection re-runs without a window reload. When something changes that none of those files record — a reinstall that leaves the lockfile untouched, or a `node_modules` that ends up broken — run **Rstack: Relaunch Extension** from the Command Palette (also on the status bar hover) to tear every tool down and start over. To rebuild a single tool, use **Rstack: Restart Rslint** / **Restart Rstest** / **Restart rs fmt**. + +A restart re-resolves every binary and package version and respawns every tool process, but it cannot reload JavaScript the editor has already imported from your project — Node keeps those modules for the lifetime of the window. If a reinstall replaced `@rslint/core` in place and lint still behaves like the old version, reload the window. Deprecated `rslint.json` / `rslint.jsonc` configs are **not** detection signals — migrate them with `rslint --init`. ## Supported package versions diff --git a/packages/vscode/src/stacks/lint/projectModules.ts b/packages/vscode/src/stacks/lint/projectModules.ts index 3bb517b..4ac83dd 100644 --- a/packages/vscode/src/stacks/lint/projectModules.ts +++ b/packages/vscode/src/stacks/lint/projectModules.ts @@ -16,6 +16,18 @@ import { pathToFileURL } from 'node:url'; * exactly the bundling seam the resolve-from-project adaptation deletes. * - Windows: only `file:`/`data:`/`node:` URLs are accepted by Node's default * ESM loader, so absolute paths are converted with `pathToFileURL`. + * + * There is deliberately no invalidation hook, and a restart does not get one. + * The memo below is the *second* cache in front of these modules: Node's own + * ESM registry is keyed by resolved URL and lives as long as the extension + * host, so once a path has loaded, re-importing it returns the identical + * module object no matter what this map says. Adding a cache-busting query to + * the specifier does reload the entry module, but a relative specifier inside + * it does not inherit the query — the result is a fresh entry wired to its own + * stale dependencies, which is worse than being consistently stale. A + * `node_modules` replaced in place under an unchanged path therefore needs a + * window reload; see the README's note on what restart does and does not + * recover. */ const cache = new Map>(); From 1e817a867a397f622c4d7fab9927e0c93a29a010 Mon Sep 17 00:00:00 2001 From: fi3ework Date: Thu, 6 Aug 2026 12:55:40 +0800 Subject: [PATCH 4/7] fix: run shell teardown on the shared queue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `retire` drops a controller from the shell's map before awaiting its teardown, so a restart in flight leaves a window where the map is already empty and the Rslint client is still shutting down. `dispose()` walked that map, found nothing, and went on to dispose the channels out from under the running teardown — letting deactivate() resolve while the child processes were still alive. Putting the teardown pass on the same queue as reconciles and restarts makes "whatever was in flight has finished" something dispose can wait for; the `#disposed` flag it sets first still stops that pass from rebuilding anything on its way out. The regression test asserts on the output channels rather than on deactivate's promise: "has not resolved yet" races the microtask queue, whereas "the channel this teardown still logs to is alive" is a fact. Confirmed to fail against the previous dispose(). --- packages/vscode/src/extension.test.ts | 66 ++++++++++++++++++++++++++- packages/vscode/src/extension.ts | 27 +++++++---- 2 files changed, 84 insertions(+), 9 deletions(-) diff --git a/packages/vscode/src/extension.test.ts b/packages/vscode/src/extension.test.ts index ef7f4cf..da8dfc1 100644 --- a/packages/vscode/src/extension.test.ts +++ b/packages/vscode/src/extension.test.ts @@ -21,6 +21,16 @@ const harness = rs.hoisted(() => { /** Stacks whose controller rejects in `register` / `dispose`. */ failRegister: new Set(), failDispose: new Set(), + /** + * Stacks whose `dispose` blocks until released, so a test can hold a + * teardown open and act while it is in flight — the Rslint client's + * graceful-then-forced shutdown, in miniature. + */ + blockDispose: new Map>(), + /** Stacks whose `dispose` has started but not yet returned. */ + disposing: new Set(), + /** Set once the shell tears down the output channels it owns. */ + channelsDisposed: false, /** Ordered `register:` / `dispose:` trace. */ events: [] as string[], /** One entry per detection pass the shell asked for. */ @@ -40,6 +50,12 @@ const harness = rs.hoisted(() => { }, dispose: async () => { state.events.push(`dispose:${stack}`); + const block = state.blockDispose.get(stack); + if (block) { + state.disposing.add(stack); + await block; + state.disposing.delete(stack); + } if (state.failDispose.has(stack)) { throw new Error(`${stack} refuses to dispose`); } @@ -73,7 +89,9 @@ rs.mock('vscode', () => { warn: (message: string) => record('warn', message), error: (message: string) => record('error', message), show: () => undefined, - dispose: () => undefined, + dispose: () => { + harness.channelsDisposed = true; + }, }; }; const vscode = { @@ -203,11 +221,22 @@ const run = async (command: string): Promise => { const restart = (): Promise => run('rstack.restart'); +/** + * Lets every already-scheduled continuation run. A macrotask turn drains the + * whole microtask queue behind it, so this is "whatever was going to happen + * without further input has happened" — not a guess at a tick count. + */ +const settle = (): Promise => + new Promise((resolve) => setTimeout(resolve, 0)); + describe('the shell restart command', () => { beforeEach(async () => { harness.detected = new Set(['rslint', 'rstest', 'fmt']); harness.failRegister.clear(); harness.failDispose.clear(); + harness.blockDispose.clear(); + harness.disposing.clear(); + harness.channelsDisposed = false; harness.events.length = 0; harness.refreshes = 0; harness.shellLog.length = 0; @@ -360,4 +389,39 @@ describe('the shell restart command', () => { 'register', ]); }); + + it('does not tear down shell resources while a restart is still disposing', async () => { + // `retire` drops the controller from the shell's map *before* awaiting its + // teardown, so during a restart there is a window where nothing is + // registered and the Rslint client is still shutting down. A `dispose()` + // that only walked that map finds it empty, disposes the channels out from + // under the in-flight teardown, and lets `deactivate()` resolve while the + // child processes are still alive. + // + // The assertion is on the channels rather than on deactivate's promise: + // "has not resolved yet" is a race with the microtask queue, whereas "the + // channel this teardown is still logging to is alive" is a fact. + let release = (): void => undefined; + harness.blockDispose.set( + 'rslint', + new Promise((resolve) => { + release = resolve; + }), + ); + + const restarting = restart(); + await settle(); + expect(harness.disposing.has('rslint')).toBe(true); + + const shutdown = deactivate(); + await settle(); + + expect(harness.disposing.has('rslint')).toBe(true); + expect(harness.channelsDisposed).toBe(false); + + release(); + await Promise.all([restarting, shutdown]); + expect(harness.disposing.has('rslint')).toBe(false); + expect(harness.channelsDisposed).toBe(true); + }); }); diff --git a/packages/vscode/src/extension.ts b/packages/vscode/src/extension.ts index 0ad3109..4ffb96a 100644 --- a/packages/vscode/src/extension.ts +++ b/packages/vscode/src/extension.ts @@ -421,14 +421,25 @@ class ExtensionShell { async dispose(): Promise { this.#disposed = true; - // Same shape as the restart teardown: per-stack isolation, and deactivate - // runs against VS Code's shutdown budget, so the slow ones (the Rslint - // client's graceful-then-forced kill) overlap instead of queueing. - await Promise.allSettled( - [...this.#controllers].map(([stack, controller]) => - this.retire(stack, controller), - ), - ); + // Behind the shared queue rather than beside it. `retire` drops a + // controller from `#controllers` before awaiting its teardown, so a + // restart in flight leaves a window where the map is already empty and the + // Rslint client is still shutting down: disposing the channels there would + // pull them out from under it, and `deactivate()` would resolve before the + // child processes are gone. The queue is what makes "whatever was in + // flight has finished" something this can wait for, and `#disposed` above + // stops that pass from rebuilding anything on its way out. + // + // The pass itself keeps the restart's shape: per-stack isolation, and + // deactivate runs against VS Code's shutdown budget, so the slow ones + // overlap instead of queueing. + await this.enqueue(async () => { + await Promise.allSettled( + [...this.#controllers].map(([stack, controller]) => + this.retire(stack, controller), + ), + ); + }); for (const subscription of this.#subscriptions) { subscription.dispose(); } From a98452d5bc0315e707697326d408bf1cadd29fb3 Mon Sep 17 00:00:00 2001 From: fi3ework Date: Wed, 5 Aug 2026 17:28:45 +0800 Subject: [PATCH 5/7] docs: add a roadmap to the README States, per config source, what the extension supports today and what is still coming, plus the cross-cutting items (re-detection command, bounded version ranges, retiring the standalone extensions). --- README.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/README.md b/README.md index 9ab7a8a..6797c8f 100644 --- a/README.md +++ b/README.md @@ -8,6 +8,18 @@ Rstack Editor provides unified editor support for [Rstack](https://rstack.rs), t | --- | --- | | [`packages/vscode`](./packages/vscode) | The VS Code extension (`rstack.rstack`) | +## Roadmap + +The extension takes its configuration from five sources. The tool-native configs are fully supported today; support for driving a stack from `rstack.config.*` is landing one stack at a time. + +| Config source | Status | +| --- | --- | +| `rslint.config.*` | **Supported.** Diagnostics, quick fixes and the language server, all resolved from the `@rslint/core` installed in your project. | +| `rstest.config.*` | **Supported.** Test discovery, run and debug, watch mode, coverage and snapshot updates in the Test Explorer. | +| `define.test()` in `rstack.config.*` | **Supported.** Tests run through the same config shim `rs test` uses, so the editor and the CLI resolve the config identically. | +| `define.fmt()` in `rstack.config.*` | **Planned.** Detected and reported in the status bar; formatting itself arrives next, first over `rs fmt --stdin-filepath` and later over an `rs fmt` language server. | +| `define.lint()` in `rstack.config.*` | **Planned.** Linting a project configured only through `rstack.config.*` needs upstream changes in Rslint and rstack-cli before the editor can evaluate it correctly. `rs lint` on the command line is unaffected. | + ## License Rstack Editor is licensed under the [MIT License](./LICENSE). From 67bd482b65b5268348363c9b530830a5ffea1520 Mon Sep 17 00:00:00 2001 From: fi3ework Date: Thu, 6 Aug 2026 13:30:10 +0800 Subject: [PATCH 6/7] fix: run the activation reconcile on the shared queue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `registerCommands()` runs before activation's first await, so the palette can reach a restart while activation is still bringing stacks up. The activation reconcile called `reconcile()` directly rather than through `enqueue`, so that restart ran straight through it and could retire a controller whose `register()` had not returned — the retired controller then went on to publish its exports and set `active` to true. Every pass now rides the one queue, which is what AGENTS.md already says. The regression test blocks the Rslint controller's `register()`, invokes the restart, and asserts no teardown begins for a stack whose register is still in flight. Confirmed to fail against the direct call. --- packages/vscode/src/extension.test.ts | 61 +++++++++++++++++++++++++++ packages/vscode/src/extension.ts | 7 ++- 2 files changed, 67 insertions(+), 1 deletion(-) diff --git a/packages/vscode/src/extension.test.ts b/packages/vscode/src/extension.test.ts index da8dfc1..32cc712 100644 --- a/packages/vscode/src/extension.test.ts +++ b/packages/vscode/src/extension.test.ts @@ -31,6 +31,17 @@ const harness = rs.hoisted(() => { disposing: new Set(), /** Set once the shell tears down the output channels it owns. */ channelsDisposed: false, + /** Holds the initial detection open, so a test can act during activation. */ + blockDetection: undefined as Promise | undefined, + /** Stacks whose `register` blocks until released. */ + blockRegister: new Map>(), + /** Stacks whose `register` has started but not yet returned. */ + registering: new Set(), + /** + * Stacks whose teardown began while their own `register` was still in + * flight — the signature of two passes running at once. + */ + overlaps: [] as string[], /** Ordered `register:` / `dispose:` trace. */ events: [] as string[], /** One entry per detection pass the shell asked for. */ @@ -43,6 +54,12 @@ const harness = rs.hoisted(() => { return { register: async () => { state.events.push(`register:${stack}`); + const block = state.blockRegister.get(stack); + if (block) { + state.registering.add(stack); + await block; + state.registering.delete(stack); + } if (state.failRegister.has(stack)) { throw new Error(`${stack} refuses to register`); } @@ -50,6 +67,9 @@ const harness = rs.hoisted(() => { }, dispose: async () => { state.events.push(`dispose:${stack}`); + if (state.registering.has(stack)) { + state.overlaps.push(stack); + } const block = state.blockDispose.get(stack); if (block) { state.disposing.add(stack); @@ -173,6 +193,7 @@ rs.mock('./detection', () => { return snapshot(); } async initialize() { + await harness.blockDetection; return this.snapshot; } async refresh() { @@ -237,6 +258,10 @@ describe('the shell restart command', () => { harness.blockDispose.clear(); harness.disposing.clear(); harness.channelsDisposed = false; + harness.blockDetection = undefined; + harness.blockRegister.clear(); + harness.registering.clear(); + harness.overlaps.length = 0; harness.events.length = 0; harness.refreshes = 0; harness.shellLog.length = 0; @@ -390,6 +415,42 @@ describe('the shell restart command', () => { ]); }); + it('serialises a restart invoked while activation is still running', async () => { + // `registerCommands` runs before activation's first await, so the palette + // can reach restart while activation is still bringing stacks up. If + // activation's own reconcile does not ride the queue, the restart runs + // straight through it and retires a controller whose `register()` has not + // returned — that controller then goes on to publish its exports and flip + // `active` on, having already been disposed. + await deactivate(); + harness.events.length = 0; + harness.refreshes = 0; + harness.overlaps.length = 0; + + let releaseRegister = (): void => undefined; + harness.blockRegister.set( + 'rslint', + new Promise((resolve) => { + releaseRegister = resolve; + }), + ); + + const activating = activate(context); + await settle(); + expect(harness.registering.has('rslint')).toBe(true); + + const restarting = run('rstack.restart'); + await settle(); + + // The restart must still be waiting its turn, not tearing down a stack + // that is in the middle of coming up. + expect(harness.overlaps).toEqual([]); + + releaseRegister(); + await Promise.all([activating, restarting]); + expect(harness.overlaps).toEqual([]); + }); + it('does not tear down shell resources while a restart is still disposing', async () => { // `retire` drops the controller from the shell's map *before* awaiting its // teardown, so during a restart there is a window where nothing is diff --git a/packages/vscode/src/extension.ts b/packages/vscode/src/extension.ts index 4ffb96a..faad9c0 100644 --- a/packages/vscode/src/extension.ts +++ b/packages/vscode/src/extension.ts @@ -94,7 +94,12 @@ class ExtensionShell { ); await this.#detection.initialize(); - await this.reconcile(); + // On the queue, not beside it. `registerCommands` runs before this method's + // first await, so a restart can be invoked from the palette while detection + // is still initialising — and it would then race this pass, with both + // finding an empty controller map and building a second controller that + // nothing owns. + await this.enqueue(() => this.reconcile()); void maybePromptForMigration(this.context, this.#channels.shell); } From 6c1780bfe5695654c2ab22af30094eae9e56129a Mon Sep 17 00:00:00 2001 From: fi3ework Date: Thu, 6 Aug 2026 14:27:10 +0800 Subject: [PATCH 7/7] refactor(vscode): make the shell's queue discipline structural The two concurrency fixes in this PR both came from a pass that bypassed the shell's single queue, so encode the rule instead of restating it: - `reconcile()` is now the queued wrapper and `runReconcile()` the body, matching the existing `restart()`/`runRestart()` pair. The `run*` prefix marks "already owns the queue" at every call site. - `retireAll()` replaces the two hand-rolled `Promise.allSettled` loops in `runRestart` and `dispose`. - `dispose()` drops the detection service before waiting on the queue, so a file touched during shutdown cannot arm a fresh pass behind it. The reconcileStack comments still described the pre-fix model where a teardown could run beside a reconcile; they now describe what actually happens. On the test side: drop the dead `blockDetection` field, rebuild harness state from a factory so a new field cannot leak between tests, and use `Promise.withResolvers`. --- packages/vscode/src/extension.test.ts | 65 ++++++++-------------- packages/vscode/src/extension.ts | 79 +++++++++++++++------------ 2 files changed, 68 insertions(+), 76 deletions(-) diff --git a/packages/vscode/src/extension.test.ts b/packages/vscode/src/extension.test.ts index 32cc712..c18ba1f 100644 --- a/packages/vscode/src/extension.test.ts +++ b/packages/vscode/src/extension.test.ts @@ -15,7 +15,12 @@ interface FakeController { } const harness = rs.hoisted(() => { - const state = { + /** + * Every field a test may set or read, rebuilt between tests. A factory rather + * than a list of resets in `beforeEach`, so a field added here cannot leak + * into the next test by being forgotten there. + */ + const defaults = () => ({ /** Stacks detection currently reports as detected. */ detected: new Set(), /** Stacks whose controller rejects in `register` / `dispose`. */ @@ -31,8 +36,6 @@ const harness = rs.hoisted(() => { disposing: new Set(), /** Set once the shell tears down the output channels it owns. */ channelsDisposed: false, - /** Holds the initial detection open, so a test can act during activation. */ - blockDetection: undefined as Promise | undefined, /** Stacks whose `register` blocks until released. */ blockRegister: new Map>(), /** Stacks whose `register` has started but not yet returned. */ @@ -50,6 +53,12 @@ const harness = rs.hoisted(() => { shellLog: [] as string[], commands: new Map unknown>(), contextKeys: new Map(), + }); + const state = { + ...defaults(), + reset(): void { + Object.assign(state, defaults()); + }, controller(stack: string): FakeController { return { register: async () => { @@ -193,7 +202,6 @@ rs.mock('./detection', () => { return snapshot(); } async initialize() { - await harness.blockDetection; return this.snapshot; } async refresh() { @@ -252,21 +260,8 @@ const settle = (): Promise => describe('the shell restart command', () => { beforeEach(async () => { + harness.reset(); harness.detected = new Set(['rslint', 'rstest', 'fmt']); - harness.failRegister.clear(); - harness.failDispose.clear(); - harness.blockDispose.clear(); - harness.disposing.clear(); - harness.channelsDisposed = false; - harness.blockDetection = undefined; - harness.blockRegister.clear(); - harness.registering.clear(); - harness.overlaps.length = 0; - harness.events.length = 0; - harness.refreshes = 0; - harness.shellLog.length = 0; - harness.commands.clear(); - harness.contextKeys.clear(); await activate(context); expect(stacksOf('register')).toEqual(['fmt', 'rslint', 'rstest']); harness.events.length = 0; @@ -427,48 +422,36 @@ describe('the shell restart command', () => { harness.refreshes = 0; harness.overlaps.length = 0; - let releaseRegister = (): void => undefined; - harness.blockRegister.set( - 'rslint', - new Promise((resolve) => { - releaseRegister = resolve; - }), - ); + const blockedRegister = Promise.withResolvers(); + harness.blockRegister.set('rslint', blockedRegister.promise); const activating = activate(context); await settle(); expect(harness.registering.has('rslint')).toBe(true); - const restarting = run('rstack.restart'); + const restarting = restart(); await settle(); // The restart must still be waiting its turn, not tearing down a stack // that is in the middle of coming up. expect(harness.overlaps).toEqual([]); - releaseRegister(); + blockedRegister.resolve(); await Promise.all([activating, restarting]); expect(harness.overlaps).toEqual([]); }); it('does not tear down shell resources while a restart is still disposing', async () => { - // `retire` drops the controller from the shell's map *before* awaiting its - // teardown, so during a restart there is a window where nothing is - // registered and the Rslint client is still shutting down. A `dispose()` - // that only walked that map finds it empty, disposes the channels out from - // under the in-flight teardown, and lets `deactivate()` resolve while the - // child processes are still alive. + // Covers the window `dispose()` documents: mid-restart the controller map + // is already empty while the Rslint client is still shutting down, so a + // teardown that only walked that map would pull the channels out from + // under it. // // The assertion is on the channels rather than on deactivate's promise: // "has not resolved yet" is a race with the microtask queue, whereas "the // channel this teardown is still logging to is alive" is a fact. - let release = (): void => undefined; - harness.blockDispose.set( - 'rslint', - new Promise((resolve) => { - release = resolve; - }), - ); + const blockedDispose = Promise.withResolvers(); + harness.blockDispose.set('rslint', blockedDispose.promise); const restarting = restart(); await settle(); @@ -480,7 +463,7 @@ describe('the shell restart command', () => { expect(harness.disposing.has('rslint')).toBe(true); expect(harness.channelsDisposed).toBe(false); - release(); + blockedDispose.resolve(); await Promise.all([restarting, shutdown]); expect(harness.disposing.has('rslint')).toBe(false); expect(harness.channelsDisposed).toBe(true); diff --git a/packages/vscode/src/extension.ts b/packages/vscode/src/extension.ts index faad9c0..b6bed7f 100644 --- a/packages/vscode/src/extension.ts +++ b/packages/vscode/src/extension.ts @@ -96,10 +96,9 @@ class ExtensionShell { await this.#detection.initialize(); // On the queue, not beside it. `registerCommands` runs before this method's // first await, so a restart can be invoked from the palette while detection - // is still initialising — and it would then race this pass, with both - // finding an empty controller map and building a second controller that - // nothing owns. - await this.enqueue(() => this.reconcile()); + // is still initialising — and running beside it would let that restart + // retire a controller whose `register()` has not returned yet. + await this.reconcile(); void maybePromptForMigration(this.context, this.#channels.shell); } @@ -185,11 +184,20 @@ class ExtensionShell { return pass; } + /** + * Queues a reconcile. The `run*` methods below are the bodies of a pass and + * assume they already own the queue — going through `enqueue` from inside + * one would wait on itself. Everything else calls the wrappers. + */ + private reconcile(stacks?: readonly StackId[]): Promise { + return this.enqueue(() => this.runReconcile(stacks)); + } + private scheduleReconcile(): void { if (this.#disposed) { return; } - void this.enqueue(() => this.reconcile()); + void this.reconcile(); } /** @@ -219,15 +227,7 @@ class ExtensionShell { this.#channels.shell.info( `Restarting ${what}${reason ? ` (${reason})` : ''}`, ); - // Per-stack isolation covers the restart too: a stack throwing on the way - // down must not keep the others from coming back up. - await Promise.allSettled( - [...this.#controllers] - .filter(([stack]) => stacks.includes(stack)) - .map(([stack, controller]) => - this.retire(stack, controller, { kind: 'starting' }), - ), - ); + await this.retireAll(stacks, { kind: 'starting' }); // Teardown is slow (closing the Rslint language client, `$close()`ing the // Rstest workers), so a `deactivate()` can land inside it. From here on // every shell resource — the output channels above all — may already be @@ -248,12 +248,28 @@ class ExtensionShell { ); } } - await this.reconcile(stacks); + await this.runReconcile(stacks); if (!this.#disposed) { this.#channels.shell.info(`${what} restart finished`); } } + /** + * Retires whichever of `stacks` are registered. Per-stack isolation covers + * teardown too: one throwing on the way down must not keep the others from + * coming back up, or from being collected at all. + */ + private async retireAll( + stacks: readonly StackId[], + next?: StackState, + ): Promise { + await Promise.allSettled( + [...this.#controllers] + .filter(([stack]) => stacks.includes(stack)) + .map(([stack, controller]) => this.retire(stack, controller, next)), + ); + } + /** * Retires a controller. Only the state left on the status bar says why * (`starting` for a restart about to rebuild it, the gate's own state when it @@ -275,7 +291,7 @@ class ExtensionShell { this.#statusBar.setState(stack, next); } - private async reconcile( + private async runReconcile( stacks: readonly StackId[] = STACK_IDS, ): Promise { if (this.#disposed) { @@ -296,10 +312,10 @@ class ExtensionShell { `rstack.${stack}.detected`, snapshot.isDetected(stack), ); - // Setting a context key is a round trip to the main thread, and a restart - // empties `#controllers` before getting here — so a `deactivate()` landing - // in this window would find nothing to dispose and the controller built - // below would outlive the extension. + // `#disposed` flips outside the queue, so it can become true mid-pass even + // though the teardown that follows it cannot start until this pass ends. + // Bailing here is the optimisation, not the safety net: anything this pass + // did register lands in `#controllers` and the queued teardown collects it. if (this.#disposed) { return; } @@ -334,9 +350,9 @@ class ExtensionShell { onDidChangeDetection: this.#detectionEmitter.event, requestRestart: (reason) => this.restart(stack, reason), }); - // `register()` is the long await here (an LSP handshake, a worker spawn), - // so `dispose()` can have run through its controller loop while this one - // was still starting. Nothing else will collect it — do it here. + // Retiring it here rather than leaving it to the queued teardown skips + // publishing exports and flipping `active` on for a stack the extension + // is already shutting down. if (this.#disposed) { await this.retire(stack, controller); return; @@ -426,6 +442,10 @@ class ExtensionShell { async dispose(): Promise { this.#disposed = true; + // Before the wait below, not after it: the service holds a debounce timer + // and its own watchers, so leaving it live means a file touched during + // shutdown can start a fresh detection pass behind us. + this.#detection.dispose(); // Behind the shared queue rather than beside it. `retire` drops a // controller from `#controllers` before awaiting its teardown, so a // restart in flight leaves a window where the map is already empty and the @@ -434,23 +454,12 @@ class ExtensionShell { // child processes are gone. The queue is what makes "whatever was in // flight has finished" something this can wait for, and `#disposed` above // stops that pass from rebuilding anything on its way out. - // - // The pass itself keeps the restart's shape: per-stack isolation, and - // deactivate runs against VS Code's shutdown budget, so the slow ones - // overlap instead of queueing. - await this.enqueue(async () => { - await Promise.allSettled( - [...this.#controllers].map(([stack, controller]) => - this.retire(stack, controller), - ), - ); - }); + await this.enqueue(() => this.retireAll(STACK_IDS)); for (const subscription of this.#subscriptions) { subscription.dispose(); } this.#subscriptions.length = 0; this.#detectionEmitter.dispose(); - this.#detection.dispose(); this.#statusBar.dispose(); this.#channels.dispose(); }