Skip to content

Commit 84666e1

Browse files
authored
fix(plan): time share by approval mode dashboard reporting negative time shares (google-gemini#19847)
1 parent a7d8511 commit 84666e1

File tree

2 files changed

+93
-9
lines changed

2 files changed

+93
-9
lines changed

packages/core/src/config/config.test.ts

Lines changed: 83 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,10 @@ import { ShellTool } from '../tools/shell.js';
3434
import { ReadFileTool } from '../tools/read-file.js';
3535
import { GrepTool } from '../tools/grep.js';
3636
import { RipGrepTool, canUseRipgrep } from '../tools/ripGrep.js';
37-
import { logRipgrepFallback } from '../telemetry/loggers.js';
37+
import {
38+
logRipgrepFallback,
39+
logApprovalModeDuration,
40+
} from '../telemetry/loggers.js';
3841
import { RipgrepFallbackEvent } from '../telemetry/types.js';
3942
import { ToolRegistry } from '../tools/tool-registry.js';
4043
import { ACTIVATE_SKILL_TOOL_NAME } from '../tools/tool-names.js';
@@ -131,6 +134,7 @@ vi.mock('../telemetry/loggers.js', async (importOriginal) => {
131134
return {
132135
...actual,
133136
logRipgrepFallback: vi.fn(),
137+
logApprovalModeDuration: vi.fn(),
134138
};
135139
});
136140

@@ -1419,6 +1423,84 @@ describe('setApprovalMode with folder trust', () => {
14191423
expect(updateSpy).not.toHaveBeenCalled();
14201424
});
14211425

1426+
describe('approval mode duration logging', () => {
1427+
beforeEach(() => {
1428+
vi.mocked(logApprovalModeDuration).mockClear();
1429+
});
1430+
1431+
it('should initialize lastModeSwitchTime with performance.now() and log positive duration', () => {
1432+
const startTime = 1000;
1433+
const endTime = 5000;
1434+
const performanceSpy = vi.spyOn(performance, 'now');
1435+
1436+
performanceSpy.mockReturnValueOnce(startTime);
1437+
const config = new Config(baseParams);
1438+
vi.spyOn(config, 'isTrustedFolder').mockReturnValue(true);
1439+
1440+
performanceSpy.mockReturnValueOnce(endTime);
1441+
config.setApprovalMode(ApprovalMode.PLAN);
1442+
1443+
expect(logApprovalModeDuration).toHaveBeenCalledWith(
1444+
config,
1445+
expect.objectContaining({
1446+
mode: ApprovalMode.DEFAULT,
1447+
duration_ms: endTime - startTime,
1448+
}),
1449+
);
1450+
performanceSpy.mockRestore();
1451+
});
1452+
1453+
it('should skip logging if duration is zero or negative', () => {
1454+
const startTime = 5000;
1455+
const endTime = 4000;
1456+
const performanceSpy = vi.spyOn(performance, 'now');
1457+
1458+
performanceSpy.mockReturnValueOnce(startTime);
1459+
const config = new Config(baseParams);
1460+
vi.spyOn(config, 'isTrustedFolder').mockReturnValue(true);
1461+
1462+
performanceSpy.mockReturnValueOnce(endTime);
1463+
config.setApprovalMode(ApprovalMode.PLAN);
1464+
1465+
expect(logApprovalModeDuration).not.toHaveBeenCalled();
1466+
performanceSpy.mockRestore();
1467+
});
1468+
1469+
it('should update lastModeSwitchTime after logging to prevent double counting', () => {
1470+
const time1 = 1000;
1471+
const time2 = 3000;
1472+
const time3 = 6000;
1473+
const performanceSpy = vi.spyOn(performance, 'now');
1474+
1475+
performanceSpy.mockReturnValueOnce(time1);
1476+
const config = new Config(baseParams);
1477+
vi.spyOn(config, 'isTrustedFolder').mockReturnValue(true);
1478+
1479+
performanceSpy.mockReturnValueOnce(time2);
1480+
config.setApprovalMode(ApprovalMode.PLAN);
1481+
expect(logApprovalModeDuration).toHaveBeenCalledWith(
1482+
config,
1483+
expect.objectContaining({
1484+
mode: ApprovalMode.DEFAULT,
1485+
duration_ms: time2 - time1,
1486+
}),
1487+
);
1488+
1489+
vi.mocked(logApprovalModeDuration).mockClear();
1490+
1491+
performanceSpy.mockReturnValueOnce(time3);
1492+
config.setApprovalMode(ApprovalMode.YOLO);
1493+
expect(logApprovalModeDuration).toHaveBeenCalledWith(
1494+
config,
1495+
expect.objectContaining({
1496+
mode: ApprovalMode.PLAN,
1497+
duration_ms: time3 - time2,
1498+
}),
1499+
);
1500+
performanceSpy.mockRestore();
1501+
});
1502+
});
1503+
14221504
describe('registerCoreTools', () => {
14231505
beforeEach(() => {
14241506
vi.clearAllMocks();

packages/core/src/config/config.ts

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -699,7 +699,7 @@ export class Config {
699699
private terminalBackground: string | undefined = undefined;
700700
private remoteAdminSettings: AdminControlsSettings | undefined;
701701
private latestApiRequest: GenerateContentParameters | undefined;
702-
private lastModeSwitchTime: number = Date.now();
702+
private lastModeSwitchTime: number = performance.now();
703703
readonly userHintService: UserHintService;
704704
private approvedPlanPath: string | undefined;
705705

@@ -1798,12 +1798,11 @@ export class Config {
17981798

17991799
const currentMode = this.getApprovalMode();
18001800
if (currentMode !== mode) {
1801-
this.logCurrentModeDuration(this.getApprovalMode());
1801+
this.logCurrentModeDuration(currentMode);
18021802
logApprovalModeSwitch(
18031803
this,
18041804
new ApprovalModeSwitchEvent(currentMode, mode),
18051805
);
1806-
this.lastModeSwitchTime = Date.now();
18071806
}
18081807

18091808
this.policyEngine.setApprovalMode(mode);
@@ -1866,12 +1865,15 @@ export class Config {
18661865
* Logs the duration of the current approval mode.
18671866
*/
18681867
logCurrentModeDuration(mode: ApprovalMode): void {
1869-
const now = Date.now();
1868+
const now = performance.now();
18701869
const duration = now - this.lastModeSwitchTime;
1871-
logApprovalModeDuration(
1872-
this,
1873-
new ApprovalModeDurationEvent(mode, duration),
1874-
);
1870+
if (duration > 0) {
1871+
logApprovalModeDuration(
1872+
this,
1873+
new ApprovalModeDurationEvent(mode, duration),
1874+
);
1875+
}
1876+
this.lastModeSwitchTime = now;
18751877
}
18761878

18771879
isYoloModeDisabled(): boolean {

0 commit comments

Comments
 (0)