diff --git a/.changeset/faster-tab-completion.md b/.changeset/faster-tab-completion.md new file mode 100644 index 0000000000..f6b253e784 --- /dev/null +++ b/.changeset/faster-tab-completion.md @@ -0,0 +1,5 @@ +--- +"@fission-ai/openspec": patch +--- + +Shell Tab completion is faster. `openspec __complete`, which the completion scripts run on every Tab press, now loads 48 modules instead of 344 when completing changes or specs, and no longer records usage, so a press never waits on the telemetry request and can't use up the first-run telemetry notice where nobody sees it. Completion output is unchanged. diff --git a/src/cli/index.ts b/src/cli/index.ts index 3501d0f5bf..4504e44432 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -123,6 +123,14 @@ export function isCompletionRun(commandPath: string): boolean { return commandPath.split(':')[0] === 'completion' || commandPath === '__complete'; } +/** + * True for the hidden `__complete` resolver alone. It runs on every Tab press, + * so the hooks skip telemetry and the completion tip for it entirely. + */ +export function isTabCompletionRun(commandPath: string): boolean { + return commandPath === '__complete'; +} + /** * True when the first-run completions tip must be deferred rather than shown. * @@ -196,6 +204,13 @@ program.hook('preAction', async (thisCommand, actionCommand) => { process.env.NO_COLOR = '1'; } + // `__complete` runs on every Tab press, with stderr sent to /dev/null by the + // generated scripts. Tracking it would hold each press open for the request, + // and the first-run notice would be marked seen without anyone reading it. + if (isTabCompletionRun(getCommandPath(actionCommand))) { + return; + } + // Show first-run telemetry notice (if not seen). It's written to stderr, so it // never pollutes stdout — but --json runs still defer it (see isJsonRun) so the // very first invocation stays free of any incidental output on either stream. @@ -210,6 +225,12 @@ program.hook('preAction', async (thisCommand, actionCommand) => { // Shutdown telemetry after command completes program.hook('postAction', async (_thisCommand, actionCommand) => { + // Nothing to flush (preAction tracked nothing), and the tip is always + // deferred for completion runs, so skip loading either module. + if (isTabCompletionRun(getCommandPath(actionCommand))) { + return; + } + // Show the first-run shell-completions tip (on stderr, so piped stdout stays // clean). postAction, not preAction: the tip trails the command's own output // instead of pushing an error message or `init`'s setup summary down the diff --git a/src/commands/completion.ts b/src/commands/completion.ts index a0487e5740..89f3c46d1c 100644 --- a/src/commands/completion.ts +++ b/src/commands/completion.ts @@ -1,4 +1,3 @@ -import ora from 'ora'; import { CompletionFactory } from '../core/completions/factory.js'; import { COMMAND_REGISTRY } from '../core/completions/command-registry.js'; import { detectShell, SupportedShell } from '../utils/shell-detection.js'; @@ -125,6 +124,9 @@ export class CompletionCommand { const generator = CompletionFactory.createGenerator(shell); const installer = CompletionFactory.createInstaller(shell); + // Loaded here, not at the top: `__complete` shares this module and runs on + // every Tab press, so it must not pay for the spinner. + const { default: ora } = await import('ora'); const spinner = ora(`Installing ${shell} completion script...`).start(); try { @@ -234,6 +236,7 @@ export class CompletionCommand { } } + const { default: ora } = await import('ora'); const spinner = ora(`Uninstalling ${shell} completion script...`).start(); try { diff --git a/src/core/completions/completion-provider.ts b/src/core/completions/completion-provider.ts index 0159131486..75606f4e0f 100644 --- a/src/core/completions/completion-provider.ts +++ b/src/core/completions/completion-provider.ts @@ -1,5 +1,4 @@ import { getActiveChangeIds, getSpecIds } from '../../utils/item-discovery.js'; -import { listSchemas } from '../artifact-graph/index.js'; /** * Cache entry for completion data @@ -96,7 +95,10 @@ export class CompletionProvider { return this.schemaCache.data; } - // Fetch fresh data + // Fetch fresh data. The resolver (and the yaml and zod it parses schemas + // with) loads only here: completing changes or specs never needs it, and + // the artifact-graph barrel would also pull in fast-glob. + const { listSchemas } = await import('../artifact-graph/resolver.js'); const schemaNames = listSchemas(this.projectRoot); // Update cache diff --git a/test/cli-e2e/startup-modules.test.ts b/test/cli-e2e/startup-modules.test.ts index 3eadf4d913..2ee546d5dc 100644 --- a/test/cli-e2e/startup-modules.test.ts +++ b/test/cli-e2e/startup-modules.test.ts @@ -1,6 +1,6 @@ import { afterAll, describe, expect, it } from 'vitest'; import { spawnSync } from 'node:child_process'; -import { mkdtempSync, readFileSync, realpathSync, rmSync } from 'node:fs'; +import { existsSync, mkdtempSync, readFileSync, realpathSync, rmSync } from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import { fileURLToPath, pathToFileURL } from 'node:url'; @@ -118,6 +118,101 @@ describe('CLI startup loads only what the command needs', () => { } ); + // Shell completion scripts run `__complete` on every Tab press. It needs no + // package beyond commander, and never telemetry: tracking would hold each + // press open for the request. + it.each(['changes', 'specs'])( + '`openspec __complete %s` loads only commander and no telemetry', + async (type) => { + await ensureCliBuilt(); + const { own, packages, status } = loadedModules(['__complete', type]); + + expect(status).toBe(0); + expect([...packages]).toEqual(['commander']); + expect(implementationsLoaded(own)).toEqual(['completion']); + expect(own).not.toContain('telemetry/index.js'); + } + ); + + it('`openspec __complete schemas` loads the schema reader but not fast-glob or ora', async () => { + await ensureCliBuilt(); + const { own, packages, status } = loadedModules(['__complete', 'schemas']); + + expect(status).toBe(0); + expect([...packages]).not.toContain('fast-glob'); + expect([...packages]).not.toContain('ora'); + expect(own).not.toContain('telemetry/index.js'); + // The name promises a schema reader, so pin it: the resolver directly, not + // the artifact-graph barrel, which re-exports it but also pulls outputs.js + // and with it fast-glob. + expect(own).toContain('core/artifact-graph/resolver.js'); + expect(own).not.toContain('core/artifact-graph/index.js'); + }); + + // Negative control for the two cases above. `isTabCompletionRun` matches the + // command path exactly, unlike the adjacent `isCompletionRun`, which + // prefix-matches `completion:*`. Widening one into the other would silently + // stop tracking every user-facing command in that group, and every assertion + // above would still pass. + it.each([['list --json'], ['completion generate zsh']])( + '`openspec %s` still reaches the telemetry hooks', + async (invocation) => { + await ensureCliBuilt(); + const { own } = loadedModules(invocation.split(' ')); + + expect(own).toContain('telemetry/index.js'); + expect(own).toContain('core/completion-tip.js'); + } + ); + + // The module-count assertions above pin that telemetry is not LOADED. They do + // not pin the user-visible consequence, which is the reason the skip exists: + // the first-run disclosure goes to stderr, every generated completion script + // sends `__complete` stderr to /dev/null, and `maybeShowTelemetryNotice` marks + // `noticeSeen` before `trackCommand` re-reads it. So before this change a + // user's first-ever interaction being a Tab press burned the disclosure where + // nobody could read it and began collecting on that same press. + // + // Telemetry is deliberately left ENABLED here: the point is that an enabled + // run still records nothing. HOME must be isolated as well as XDG_CONFIG_HOME, + // or `migrateLegacyTelemetryConfig` imports the developer's real config from + // ~/.config/openspec/config.json and the test passes vacuously. + it('`openspec __complete` leaves the first-run telemetry notice unconsumed', async () => { + await ensureCliBuilt(); + const home = mkdtempSync(path.join(os.tmpdir(), 'openspec-notice-')); + workDirs.push(home); + const configHome = path.join(home, 'config'); + + // These have to be DELETED, not blanked. Both opt-outs read as "set to + // anything that is not an explicit on-value", so `OPENSPEC_TELEMETRY: ''` + // and `DO_NOT_TRACK: ''` would switch telemetry OFF and the assertions + // below would hold on any build, passing vacuously. + const env = { + ...process.env, + HOME: home, + USERPROFILE: home, + XDG_CONFIG_HOME: configHome, + XDG_DATA_HOME: path.join(home, 'data'), + }; + delete env.OPENSPEC_TELEMETRY; + delete env.DO_NOT_TRACK; + delete env.CI; + + const result = spawnSync(process.execPath, [binPath, '__complete', 'changes'], { + cwd: home, + env, + encoding: 'utf-8', + timeout: 30_000, + windowsHide: true, + }); + + expect(result.status).toBe(0); + // No disclosure written to a stream the completion scripts discard. + expect(result.stderr).toBe(''); + // And no config file, so neither noticeSeen nor an anonymousId was minted. + expect(existsSync(path.join(configHome, 'openspec', 'config.json'))).toBe(false); + }); + it.each([ ['config list --json', 'config'], ['config path', 'config'], diff --git a/test/core/cli-is-json-run.test.ts b/test/core/cli-is-json-run.test.ts index 420dcf57cd..2b7a9b722b 100644 --- a/test/core/cli-is-json-run.test.ts +++ b/test/core/cli-is-json-run.test.ts @@ -1,7 +1,12 @@ import { describe, it, expect } from 'vitest'; import { Command, Option } from 'commander'; -import { isJsonRun, isCompletionRun, shouldDeferCompletionTip } from '../../src/cli/index.js'; +import { + isJsonRun, + isCompletionRun, + isTabCompletionRun, + shouldDeferCompletionTip, +} from '../../src/cli/index.js'; /** * Reproduce the three ways `--json` reaches a command in the real CLI, so a @@ -106,6 +111,23 @@ describe('isCompletionRun', () => { ); }); +describe('isTabCompletionRun', () => { + /** + * Only the hidden resolver skips telemetry. The user-facing `completion` + * commands are run by a person and are tracked like any other command. + */ + it('matches the hidden resolver', () => { + expect(isTabCompletionRun('__complete')).toBe(true); + }); + + it.each(['completion', 'completion:install', 'completion:generate', 'list'])( + 'does not match "%s"', + (commandPath) => { + expect(isTabCompletionRun(commandPath)).toBe(false); + } + ); +}); + describe('shouldDeferCompletionTip', () => { /** * The tip must survive every run that cannot display it. Deferring (rather diff --git a/test/core/completions/generators/zsh-generator.test.ts b/test/core/completions/generators/zsh-generator.test.ts index 376f96350e..2f8425d222 100644 --- a/test/core/completions/generators/zsh-generator.test.ts +++ b/test/core/completions/generators/zsh-generator.test.ts @@ -421,5 +421,35 @@ describe('ZshGenerator', () => { expect(script).toContain('_openspec_view() {'); expect(script).toContain('_arguments'); }); + + // The bash, fish and powershell generator tests each pin this redirect; + // zsh was the one that did not, and it has the most `__complete` call + // sites and is the default shell on macOS. The redirect is what keeps a + // Tab press silent, so anything `__complete` ever writes to stderr - a + // deprecation notice, a Node experimental warning - would otherwise print + // into the middle of the user's prompt with no test objecting. + it('sends every __complete call stderr to /dev/null', () => { + const commands: CommandDefinition[] = [ + { name: 'archive', description: 'Archive a change', flags: [] }, + { name: 'show', description: 'Show an item', flags: [] }, + { name: 'new', description: 'Create a change', flags: [] }, + ]; + + const script = generator.generate(commands); + + expect(script).toContain('openspec __complete changes 2>/dev/null'); + expect(script).toContain('openspec __complete specs 2>/dev/null'); + expect(script).toContain('openspec __complete schemas 2>/dev/null'); + // No invocation may survive a template edit that drops a redirect. Only + // real call sites count: the script also explains itself in comments + // that name the command without running it. + const invocations = script + .split('\n') + .filter((line) => line.includes('openspec __complete') && !line.trim().startsWith('#')); + expect(invocations.length).toBeGreaterThan(0); + for (const line of invocations) { + expect(line, `unredirected __complete call: ${line.trim()}`).toContain('2>/dev/null'); + } + }); }); });