-
Notifications
You must be signed in to change notification settings - Fork 2.6k
perf(cli): defer startup prefetch tasks #6303
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 16 commits
9c472cf
66b9b0b
5f8036b
e441f64
9e67e80
1957471
006935b
3257422
d311854
64119f7
be22286
03467db
6cba2c5
d053590
02e5be8
ede844d
95c5f6c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2050,6 +2050,9 @@ export async function loadCliConfig( | |
| showResponseTokensPerSecond: | ||
| settings.ui?.showResponseTokensPerSecond === true, | ||
| telemetry: telemetrySettings, | ||
| // Prompt-interactive (`qwen -i "prompt"`) auto-submits its first prompt, | ||
| // so telemetry must be ready before that request is sent. | ||
| deferTelemetryInitialization: interactive && !isAcpMode && !question, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Two accepted tradeoffs converge at this line, and it would help future readers to document both:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in I documented the accepted telemetry deferral tradeoffs at the |
||
| outboundCorrelation: settings.outboundCorrelation, | ||
| usageStatisticsEnabled: settings.privacy?.usageStatisticsEnabled ?? true, | ||
| clearContextOnIdle: settings.context?.clearContextOnIdle, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ | |
| */ | ||
|
|
||
| import { beforeEach, describe, expect, it, vi } from 'vitest'; | ||
| import { initializeApp } from './initializer.js'; | ||
| import { connectIdeForStartup, initializeApp } from './initializer.js'; | ||
|
|
||
| const mockPerformInitialAuth = vi.fn(); | ||
| const mockValidateTheme = vi.fn(); | ||
|
|
@@ -164,7 +164,7 @@ describe('initializeApp', () => { | |
| expect(result.shouldOpenAuthDialog).toBe(false); | ||
| }); | ||
|
|
||
| it('should connect to IDE when in IDE mode', async () => { | ||
| it('should connect to IDE by default when in IDE mode', async () => { | ||
| mockConfig.getIdeMode.mockReturnValue(true); | ||
|
|
||
| await initializeApp(mockConfig as never, mockSettings as never); | ||
|
|
@@ -174,6 +174,28 @@ describe('initializeApp', () => { | |
| expect(mockLogIdeConnection).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should not connect to IDE when deferred', async () => { | ||
| mockConfig.getIdeMode.mockReturnValue(true); | ||
|
|
||
| await initializeApp(mockConfig as never, mockSettings as never, { | ||
| deferIdeConnection: true, | ||
| }); | ||
|
|
||
| expect(mockGetInstance).not.toHaveBeenCalled(); | ||
| expect(mockConnect).not.toHaveBeenCalled(); | ||
| expect(mockLogIdeConnection).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should connect to IDE through startup helper', async () => { | ||
| mockConfig.getIdeMode.mockReturnValue(true); | ||
|
|
||
| await connectIdeForStartup(mockConfig as never); | ||
|
|
||
| expect(mockGetInstance).toHaveBeenCalled(); | ||
| expect(mockConnect).toHaveBeenCalled(); | ||
| expect(mockLogIdeConnection).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should not connect to IDE when not in IDE mode', async () => { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion]
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in I added a direct |
||
| mockConfig.getIdeMode.mockReturnValue(false); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] This test verifies
deferTelemetryInitialization: falsefor prompt-interactive, but there is no corresponding positive test verifyingdeferTelemetryInitialization: truefor the ordinary interactive case (no-p, no-i "prompt", not ACP). The positive path is the primary motivation for the feature — deferring telemetry for ordinary TUI startup — yet it has no test. A regression that accidentally always setsfalsewould go undetected.Consider adding a test with
process.argv = ['node', 'script.js', '-i'](interactive, no question) that assertsdeferTelemetryInitialization: true.— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in
6cba2c578.Added a positive
loadCliConfigtest for ordinary interactive startup that setsprocess.stdin.isTTY = true, uses no initial prompt, and assertsdeferTelemetryInitialization: trueis passed toConfig.