From 0a6eb4361d37ba85a0ef905552b6d9af3be6039b Mon Sep 17 00:00:00 2001 From: Tyler Coles Date: Sun, 1 Mar 2026 10:06:13 -0500 Subject: [PATCH] fix: NaN averages, state mutation via push, TreeView selection clearing, formatBytes underflow, refreshInterval guard --- .../src/utils/formatters.test.ts | 12 ++++ .../devtools-common/src/utils/formatters.ts | 2 +- .../src/__tests__/manager.test.ts | 25 ++++++++ packages/feature-flags/src/manager.ts | 4 +- .../src/components/TreeView.test.tsx | 25 +++++++- .../src/components/TreeView.tsx | 12 ++-- .../src/core/devtools-store.ts | 13 ++-- .../src/core/__tests__/devtools-store.test.ts | 64 +++++++++++++++++++ .../src/core/devtools-store.ts | 9 ++- 9 files changed, 145 insertions(+), 21 deletions(-) create mode 100644 plugins/graphql-devtools/src/core/__tests__/devtools-store.test.ts diff --git a/packages/devtools-common/src/utils/formatters.test.ts b/packages/devtools-common/src/utils/formatters.test.ts index ee25ccf..0e19395 100644 --- a/packages/devtools-common/src/utils/formatters.test.ts +++ b/packages/devtools-common/src/utils/formatters.test.ts @@ -45,4 +45,16 @@ describe('formatBytes', () => { it('treats negative decimals as zero', () => { expect(formatBytes(1536, -1)).toBe('2 KB'); }); + + it('handles sub-byte fractional values without undefined unit (the fix)', () => { + const result = formatBytes(0.0001); + expect(result).not.toContain('undefined'); + expect(result).toContain('B'); + }); + + it('handles very small positive values', () => { + const result = formatBytes(0.5); + expect(result).toContain('B'); + expect(result).not.toContain('undefined'); + }); }); diff --git a/packages/devtools-common/src/utils/formatters.ts b/packages/devtools-common/src/utils/formatters.ts index 9c89ecb..ed15c4e 100644 --- a/packages/devtools-common/src/utils/formatters.ts +++ b/packages/devtools-common/src/utils/formatters.ts @@ -15,7 +15,7 @@ export function formatBytes(bytes: number, decimals: number = 2): string { const dm = decimals < 0 ? 0 : decimals; const sizes = ['B', 'KB', 'MB', 'GB', 'TB', 'PB']; - const i = Math.floor(Math.log(absBytes) / Math.log(k)); + const i = Math.max(0, Math.floor(Math.log(absBytes) / Math.log(k))); return sign + parseFloat((absBytes / Math.pow(k, i)).toFixed(dm)) + ' ' + sizes[i]; } diff --git a/packages/feature-flags/src/__tests__/manager.test.ts b/packages/feature-flags/src/__tests__/manager.test.ts index 438e0f2..9e7803a 100644 --- a/packages/feature-flags/src/__tests__/manager.test.ts +++ b/packages/feature-flags/src/__tests__/manager.test.ts @@ -488,4 +488,29 @@ describe('FeatureFlagManager', () => { newManager.destroy(); }); }); + + describe('refreshInterval safety (the fix)', () => { + it('defaults refreshInterval to 30000 when explicitly passed as undefined', () => { + const mgr = new FeatureFlagManager({ + storage, + autoRefresh: false, + refreshInterval: undefined, + }); + + // Access internal options via any cast + expect((mgr as any).options.refreshInterval).toBe(30000); + mgr.destroy(); + }); + + it('uses provided refreshInterval when valid', () => { + const mgr = new FeatureFlagManager({ + storage, + autoRefresh: false, + refreshInterval: 5000, + }); + + expect((mgr as any).options.refreshInterval).toBe(5000); + mgr.destroy(); + }); + }); }); diff --git a/packages/feature-flags/src/manager.ts b/packages/feature-flags/src/manager.ts index e8b4056..37d7125 100644 --- a/packages/feature-flags/src/manager.ts +++ b/packages/feature-flags/src/manager.ts @@ -33,8 +33,8 @@ export class FeatureFlagManager { this.options = { persistOverrides: true, autoRefresh: false, - refreshInterval: 30000, - ...options + ...options, + refreshInterval: options.refreshInterval ?? 30000, }; this.storage = options.storage || diff --git a/packages/shared-components/src/components/TreeView.test.tsx b/packages/shared-components/src/components/TreeView.test.tsx index a61cb7f..a67a4d4 100644 --- a/packages/shared-components/src/components/TreeView.test.tsx +++ b/packages/shared-components/src/components/TreeView.test.tsx @@ -1,5 +1,5 @@ import React from 'react'; -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, vi } from 'vitest'; import { render, screen } from '@testing-library/react'; import { TreeView, TreeNode } from './TreeView'; @@ -130,4 +130,27 @@ describe('TreeView', () => { expect(screen.getByText('Root Node')).toBeTruthy(); expect(screen.getByText('Leaf Node')).toBeTruthy(); }); + + it('respects selectedIds=[] to clear selection (the fix)', () => { + const onSelect = vi.fn(); + // First render with a selected node + const { rerender } = render( + + ); + + // Now pass empty array to clear — should NOT fall back to internal state + rerender( + + ); + + // No node should have selection styling — verify root node is rendered but not selected + expect(screen.getByText('Root Node')).toBeTruthy(); + // The component renders — this ensures it doesn't crash with empty selectedIds + }); + + it('uses internal selection when selectedIds is not provided', () => { + render(); + expect(screen.getByText('Root Node')).toBeTruthy(); + expect(screen.getByText('Leaf Node')).toBeTruthy(); + }); }); diff --git a/packages/shared-components/src/components/TreeView.tsx b/packages/shared-components/src/components/TreeView.tsx index 0166981..a1079a3 100644 --- a/packages/shared-components/src/components/TreeView.tsx +++ b/packages/shared-components/src/components/TreeView.tsx @@ -59,7 +59,7 @@ export interface TreeViewProps { export function TreeView({ data, - selectedIds = [], + selectedIds: controlledSelectedIds, multiSelect = false, onSelect, onMultiSelect, @@ -87,7 +87,7 @@ export function TreeView({ new Set(defaultExpandedIds) ); const [internalSelectedIds, setInternalSelectedIds] = useState>( - new Set(selectedIds) + new Set(controlledSelectedIds) ); const expandedSet = useMemo(() => @@ -97,9 +97,11 @@ export function TreeView({ [controlledExpandedIds, internalExpandedIds] ); - const selectedSet = useMemo(() => - new Set(selectedIds.length > 0 ? selectedIds : internalSelectedIds), - [selectedIds, internalSelectedIds] + const selectedSet = useMemo(() => + controlledSelectedIds !== undefined + ? new Set(controlledSelectedIds) + : internalSelectedIds, + [controlledSelectedIds, internalSelectedIds] ); // Toggle expansion diff --git a/plugins/auth-permissions-mock/src/core/devtools-store.ts b/plugins/auth-permissions-mock/src/core/devtools-store.ts index c95a5fb..01c0402 100644 --- a/plugins/auth-permissions-mock/src/core/devtools-store.ts +++ b/plugins/auth-permissions-mock/src/core/devtools-store.ts @@ -219,12 +219,7 @@ class AuthMockDevToolsStore { } recordStorageOperation(operation: StorageOperation) { - this.state.storageOperations.push(operation); - - // Keep only last 100 operations - if (this.state.storageOperations.length > 100) { - this.state.storageOperations = this.state.storageOperations.slice(-100); - } + this.state.storageOperations = [...this.state.storageOperations, operation].slice(-100); this.notifyListeners(); } @@ -258,17 +253,17 @@ class AuthMockDevToolsStore { } addScenario(scenario: MockScenario) { - this.state.scenarios.push(scenario); + this.state.scenarios = [...this.state.scenarios, scenario]; this.notifyListeners(); } addRole(role: Role) { - this.state.roles.push(role); + this.state.roles = [...this.state.roles, role]; this.notifyListeners(); } addPermission(permission: Permission) { - this.state.permissions.push(permission); + this.state.permissions = [...this.state.permissions, permission]; this.notifyListeners(); } diff --git a/plugins/graphql-devtools/src/core/__tests__/devtools-store.test.ts b/plugins/graphql-devtools/src/core/__tests__/devtools-store.test.ts new file mode 100644 index 0000000..f71126b --- /dev/null +++ b/plugins/graphql-devtools/src/core/__tests__/devtools-store.test.ts @@ -0,0 +1,64 @@ +/** + * @vitest-environment jsdom + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { GraphQLDevToolsStore } from '../devtools-store'; +import type { GraphQLOperation } from '../../types'; + +function makeOperation(overrides: Partial = {}): GraphQLOperation { + return { + id: `op-${Math.random().toString(36).substring(2)}`, + operationType: 'query', + query: '{ users { id name } }', + timestamp: Date.now(), + status: 'success', + ...overrides, + }; +} + +describe('GraphQLDevToolsStore — averageExecutionTime', () => { + let store: GraphQLDevToolsStore; + + beforeEach(() => { + store = new GraphQLDevToolsStore(); + }); + + it('does not produce NaN on first operation with executionTime (the fix)', () => { + const op = makeOperation({ executionTime: 50 }); + store.dispatch({ type: 'operations/add', payload: op }); + + const state = store.getSnapshot(); + expect(Number.isNaN(state.performance.averageExecutionTime)).toBe(false); + expect(state.performance.averageExecutionTime).toBe(50); + }); + + it('calculates correct average over multiple operations', () => { + store.dispatch({ type: 'operations/add', payload: makeOperation({ executionTime: 100 }) }); + store.dispatch({ type: 'operations/add', payload: makeOperation({ executionTime: 200 }) }); + store.dispatch({ type: 'operations/add', payload: makeOperation({ executionTime: 300 }) }); + + const state = store.getSnapshot(); + expect(state.performance.averageExecutionTime).toBe(200); + }); + + it('ignores operations without executionTime', () => { + store.dispatch({ type: 'operations/add', payload: makeOperation({ executionTime: 100 }) }); + store.dispatch({ type: 'operations/add', payload: makeOperation({ executionTime: undefined }) }); + store.dispatch({ type: 'operations/add', payload: makeOperation({ executionTime: 300 }) }); + + const state = store.getSnapshot(); + // Average of [100, 300] = 200 + expect(state.performance.averageExecutionTime).toBe(200); + }); + + it('tracks slowest and fastest operations', () => { + store.dispatch({ type: 'operations/add', payload: makeOperation({ executionTime: 100 }) }); + store.dispatch({ type: 'operations/add', payload: makeOperation({ executionTime: 500 }) }); + store.dispatch({ type: 'operations/add', payload: makeOperation({ executionTime: 200 }) }); + + const state = store.getSnapshot(); + expect(state.performance.slowestOperation?.executionTime).toBe(500); + expect(state.performance.fastestOperation?.executionTime).toBe(100); + }); +}); diff --git a/plugins/graphql-devtools/src/core/devtools-store.ts b/plugins/graphql-devtools/src/core/devtools-store.ts index 35f1a1e..18257d7 100644 --- a/plugins/graphql-devtools/src/core/devtools-store.ts +++ b/plugins/graphql-devtools/src/core/devtools-store.ts @@ -468,10 +468,13 @@ export class GraphQLDevToolsStore { // Update execution time metrics if (operation.executionTime !== undefined) { - const allOperations = this.state.operations.filter(op => op.executionTime !== undefined); + // Include the current operation since this.state.operations is the pre-add snapshot + const allOperations = [...this.state.operations, operation].filter(op => op.executionTime !== undefined); const executionTimes = allOperations.map(op => op.executionTime as number); - - updated.averageExecutionTime = executionTimes.reduce((sum, time) => sum + time, 0) / executionTimes.length; + + updated.averageExecutionTime = executionTimes.length > 0 + ? executionTimes.reduce((sum, time) => sum + time, 0) / executionTimes.length + : 0; if (!updated.slowestOperation || operation.executionTime > (updated.slowestOperation.executionTime || 0)) { updated.slowestOperation = operation;