From 8b6103fc67566b4f15e37713c13c3ffa1bc347e0 Mon Sep 17 00:00:00 2001 From: Yunwen Zheng Date: Tue, 27 May 2025 14:00:47 -0400 Subject: [PATCH] TimePickerWithHistory: Improve type handling when reading history from localStorage (#105859) * TimePickerWithHistory: Improve migrateHistory type safety handling and validation --- .../TimePicker/TimePickerWithHistory.test.tsx | 31 +++--- .../TimePicker/TimePickerWithHistory.tsx | 99 ++++++++++++++----- 2 files changed, 88 insertions(+), 42 deletions(-) diff --git a/public/app/core/components/TimePicker/TimePickerWithHistory.test.tsx b/public/app/core/components/TimePicker/TimePickerWithHistory.test.tsx index 8558e5764e5..59abd9f9375 100644 --- a/public/app/core/components/TimePicker/TimePickerWithHistory.test.tsx +++ b/public/app/core/components/TimePicker/TimePickerWithHistory.test.tsx @@ -13,18 +13,6 @@ describe('TimePickerWithHistory', () => { const getApplyButton = () => screen.getByRole('button', { name: 'Apply time range' }); const LOCAL_STORAGE_KEY = 'grafana.dashboard.timepicker.history'; - const OLD_LOCAL_STORAGE = [ - { - from: '2022-12-03T00:00:00.000Z', - to: '2022-12-03T23:59:59.000Z', - raw: { from: '2022-12-03T00:00:00.000Z', to: '2022-12-03T23:59:59.000Z' }, - }, - { - from: '2022-12-02T00:00:00.000Z', - to: '2022-12-02T23:59:59.000Z', - raw: { from: '2022-12-02T00:00:00.000Z', to: '2022-12-02T23:59:59.000Z' }, - }, - ]; const NEW_LOCAL_STORAGE = [ { from: '2022-12-03T00:00:00.000Z', to: '2022-12-03T23:59:59.000Z' }, @@ -52,15 +40,28 @@ describe('TimePickerWithHistory', () => { expect(screen.getByText(/It looks like you haven't used this time picker before/i)).toBeInTheDocument(); }); - it('Should load with old TimeRange history', async () => { - window.localStorage.setItem(LOCAL_STORAGE_KEY, JSON.stringify(OLD_LOCAL_STORAGE)); + it('Should load with valid time picker history only', async () => { + // TimePickerWithHistory only accepts TimePickerHistoryItem objects, invalid history items should be ignored + const BAD_LOCAL_STORAGE = [ + { from: '2022-12-03T00:00:00.000Z', to: '2022-12-03T23:59:59.000Z' }, // valid + { + from: '2022-12-01T00:00:00.000Z', + to: '2022-12-01T23:59:59.000Z', + raw: { from: '2022-12-01T00:00:00.000Z', to: '022-12-01T23:59:59.000Z' }, // Invalid, because it has raw property which doesn't match TimePickerHistoryItem + }, + {}, // Invalid, because empty + { from: null, to: null }, // Invalid, because both value are null + { from: '2022-12-04T00:00:00.000Z', to: null }, // Invalid because one value is null + ]; + window.localStorage.setItem(LOCAL_STORAGE_KEY, JSON.stringify(BAD_LOCAL_STORAGE)); const timeRange = getDefaultTimeRange(); render(); await userEvent.click(screen.getByLabelText(/Time range selected/)); expect(screen.getByText(/2022-12-03 00:00:00 to 2022-12-03 23:59:59/i)).toBeInTheDocument(); - expect(screen.queryByText(/2022-12-02 00:00:00 to 2022-12-02 23:59:59/i)).toBeInTheDocument(); + expect(screen.queryByText(/2022-12-01 00:00:00 to 2022-12-01 23:59:59/i)).not.toBeInTheDocument(); + expect(screen.queryByText(/2022-12-04 00:00:00 to 2022-12-04 23:59:59/i)).not.toBeInTheDocument(); }); it('Should load with new TimePickerHistoryItem history', async () => { diff --git a/public/app/core/components/TimePicker/TimePickerWithHistory.tsx b/public/app/core/components/TimePicker/TimePickerWithHistory.tsx index 204f971efc0..4487eb1378a 100644 --- a/public/app/core/components/TimePicker/TimePickerWithHistory.tsx +++ b/public/app/core/components/TimePicker/TimePickerWithHistory.tsx @@ -1,6 +1,6 @@ import { uniqBy } from 'lodash'; -import { AppEvents, TimeRange, isDateTime, rangeUtil } from '@grafana/data'; +import { AppEvents, DateTime, TimeRange, isDateTime, rangeUtil } from '@grafana/data'; import { useTranslate } from '@grafana/i18n'; import { TimeRangePickerProps, TimeRangePicker } from '@grafana/ui'; import appEvents from 'app/core/app_events'; @@ -8,6 +8,7 @@ import appEvents from 'app/core/app_events'; import { LocalStorageValueProvider } from '../LocalStorageValueProvider'; const LOCAL_STORAGE_KEY = 'grafana.dashboard.timepicker.history'; +const MAX_HISTORY_ITEMS = 4; interface Props extends Omit {} @@ -17,24 +18,21 @@ interface TimePickerHistoryItem { to: string; } -// We should only be storing TimePickerHistoryItem, but in the past we also stored TimeRange -type LSTimePickerHistoryItem = TimePickerHistoryItem | TimeRange; - export const TimePickerWithHistory = (props: Props) => { const { t } = useTranslate(); return ( - storageKey={LOCAL_STORAGE_KEY} defaultValue={[]}> - {(rawValues, onSaveToStore) => { - const values = migrateHistory(rawValues); - const history = deserializeHistory(values); + storageKey={LOCAL_STORAGE_KEY} defaultValue={[]}> + {(values, onSaveToStore) => { + const validHistory = getValidHistory(values); + const history = deserializeHistory(validHistory); return ( { - onAppendToHistory(value, values, onSaveToStore); + onAppendToHistory(value, validHistory, onSaveToStore); props.onChange(value); }} onError={(error?: string) => @@ -50,21 +48,26 @@ export const TimePickerWithHistory = (props: Props) => { ); }; -function deserializeHistory(values: TimePickerHistoryItem[]): TimeRange[] { - // The history is saved in UTC and with the default date format, so we need to pass those values to the convertRawToRange - return values.map((item) => rangeUtil.convertRawToRange(item, 'utc', undefined, 'YYYY-MM-DD HH:mm:ss')); +function getValidHistory(values: unknown): TimePickerHistoryItem[] { + const result: TimePickerHistoryItem[] = []; + + if (!Array.isArray(values)) { + return result; + } + // Check if the values are already in the correct format + + for (let item of values) { + const parsed = getValidHistoryItem(item); + if (parsed) { + result.push(parsed); + } + } + + return result; } -function migrateHistory(values: LSTimePickerHistoryItem[]): TimePickerHistoryItem[] { - return values.map((item) => { - const fromValue = typeof item.from === 'string' ? item.from : item.from.toISOString(); - const toValue = typeof item.to === 'string' ? item.to : item.to.toISOString(); - - return { - from: fromValue, - to: toValue, - }; - }); +export function deserializeHistory(values: TimePickerHistoryItem[]): TimeRange[] { + return values.map((item) => rangeUtil.convertRawToRange(item, 'utc', undefined, 'YYYY-MM-DD HH:mm:ss')); } function onAppendToHistory( @@ -72,24 +75,66 @@ function onAppendToHistory( values: TimePickerHistoryItem[], onSaveToStore: (values: TimePickerHistoryItem[]) => void ) { - if (!isAbsolute(newTimeRange)) { + if (!isAbsoluteTimeRange(newTimeRange)) { + // If the time range is not absolute, do not append it to history, ex: last 5 minutes return; } // Convert DateTime objects to strings const toAppend = { - from: typeof newTimeRange.raw.from === 'string' ? newTimeRange.raw.from : newTimeRange.raw.from.toISOString(), - to: typeof newTimeRange.raw.to === 'string' ? newTimeRange.raw.to : newTimeRange.raw.to.toISOString(), + from: convertToISOString(newTimeRange.raw.from), + to: convertToISOString(newTimeRange.raw.to), }; const toStore = limit([toAppend, ...values]); onSaveToStore(toStore); } -function isAbsolute(value: TimeRange): boolean { +function isAbsoluteTimeRange(value: TimeRange): boolean { return isDateTime(value.raw.from) || isDateTime(value.raw.to); } function limit(value: TimePickerHistoryItem[]): TimePickerHistoryItem[] { - return uniqBy(value, (v) => v.from + v.to).slice(0, 4); + return uniqBy(value, (v) => v.from + v.to).slice(0, MAX_HISTORY_ITEMS); +} + +/** + * Check if the value is a valid TimePickerHistoryItem. If it doesn't match the format exactly, it will return false. + * @returns true if the value match exactly to TimePickerHistoryItem, false otherwise + */ +export function getValidHistoryItem(value: unknown): TimePickerHistoryItem | null { + // First check if it's a valid object + if (typeof value !== 'object' || value === null) { + return null; + } + + // Check if it has exactly two properties + if (Object.keys(value).length !== 2) { + return null; + } + + // Check if it has the required properties + if (!('from' in value) || !('to' in value)) { + return null; + } + + const { from, to } = value; + // Check if both properties are strings + if (typeof from === 'string' && typeof to === 'string') { + return { from, to }; + } + + return null; +} + +function convertToISOString(value: DateTime | string): string { + if (typeof value === 'string') { + return value; + } + + if (!value?.toISOString) { + throw console.error('Invalid DateTime object passed to convertToISOString'); + } + + return value.toISOString(); }