Skip to content

Commit

Permalink
fix timeline initial date range regression
Browse files Browse the repository at this point in the history
When refactoring into TimelineContext, there was a regression regarding the date range that is initially loaded.
#1142 (comment)

It used to load 7 days before the pipelineEnd up to today. After the change, it would load 7 days before today up to today.
If the user has travel everyday this ends up being equivalent. But if there isn't recent travel, this becomes a problem.

So in TimelineContext, the initial dateRange needs to depend on pipelineRange. instead of giving an initial value upfront, we'll let dateRange be null until pipelineRange is set, at which point we'll also set dateRange.
This means we have to consider some extra cases where dateRange can be null.
  • Loading branch information
JGreenlee committed Jun 7, 2024
1 parent 461e5fe commit 1955618
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 27 deletions.
53 changes: 29 additions & 24 deletions www/js/TimelineContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,7 @@ import { EnketoUserInputEntry } from './survey/enketo/enketoHelper';
import { VehicleIdentity } from './types/appConfigTypes';
import { primarySectionForTrip } from './diary/diaryHelper';

// initial query range is the past 7 days, including today
const today = DateTime.now().toISODate().substring(0, 10);
const initialQueryRange: [string, string] = [isoDateWithOffset(today, -6), today];
const TODAY_DATE = DateTime.now().toISODate();

type ContextProps = {
labelOptions: LabelOptions | null;
Expand All @@ -43,7 +41,7 @@ type ContextProps = {
addUserInputToEntry: (oid: string, userInput: any, inputType: 'label' | 'note') => void;
pipelineRange: TimestampRange | null;
queriedDateRange: [string, string] | null; // YYYY-MM-DD format
dateRange: [string, string]; // YYYY-MM-DD format
dateRange: [string, string] | null; // YYYY-MM-DD format
timelineIsLoading: string | false;
loadMoreDays: (when: 'past' | 'future', nDays: number) => boolean | void;
loadDateRange: (d: [string, string]) => boolean | void;
Expand All @@ -62,7 +60,7 @@ export const useTimelineContext = (): ContextProps => {
// date range (inclusive) that has been loaded into the UI [YYYY-MM-DD, YYYY-MM-DD]
const [queriedDateRange, setQueriedDateRange] = useState<[string, string] | null>(null);
// date range (inclusive) chosen by datepicker [YYYY-MM-DD, YYYY-MM-DD]
const [dateRange, setDateRange] = useState<[string, string]>(initialQueryRange);
const [dateRange, setDateRange] = useState<[string, string] | null>(null);
// map of timeline entries (trips, places, untracked time), ids to objects
const [timelineMap, setTimelineMap] = useState<TimelineMap | null>(null);
const [timelineIsLoading, setTimelineIsLoading] = useState<string | false>('replace');
Expand All @@ -84,26 +82,24 @@ export const useTimelineContext = (): ContextProps => {
}
}, [appConfig, refreshTime]);

// when a new date range is chosen, load more date, then update the queriedDateRange
// when a new date range is chosen, load more data, then update the queriedDateRange
useEffect(() => {
const onDateRangeChange = async () => {
if (!dateRange) return logDebug('No dateRange chosen, skipping onDateRangeChange');
if (!pipelineRange) return logDebug('No pipelineRange yet, skipping onDateRangeChange');

logDebug('Timeline: onDateRangeChange with dateRange = ' + dateRange?.join(' to '));

// determine if this will be a new range or an expansion of the existing range
let mode: 'replace' | 'prepend' | 'append';
let dateRangeToQuery = dateRange;
if (queriedDateRange?.[0] == dateRange[0] && queriedDateRange?.[1] == dateRange[1]) {
if (queriedDateRange?.[0] == dateRange?.[0] && queriedDateRange?.[1] == dateRange?.[1]) {
// same range, so we are refreshing the data
mode = 'replace';
} else if (queriedDateRange?.[0] == dateRange[0]) {
} else if (dateRange && queriedDateRange?.[0] == dateRange[0]) {
// same start date, so we are loading more data into the future
mode = 'append';
const nextDate = isoDateWithOffset(queriedDateRange[1], 1);
dateRangeToQuery = [nextDate, dateRange[1]];
} else if (queriedDateRange?.[1] == dateRange[1]) {
} else if (dateRange && queriedDateRange?.[1] == dateRange[1]) {
// same end date, so we are loading more data into the past
mode = 'prepend';
const prevDate = isoDateWithOffset(queriedDateRange[0], -1);
Expand All @@ -124,10 +120,7 @@ export const useTimelineContext = (): ContextProps => {
setTimelineIsLoading(false);
displayError(e, 'While loading date range ' + dateRange?.join(' to '));
}
}, [dateRange, pipelineRange]);

// const onDateRangeChange = useCallback(async (dateRange: [string, string], pipelineRange) => {
// }, []);
}, [dateRange]);

useEffect(() => {
if (!timelineMap) return;
Expand Down Expand Up @@ -158,20 +151,33 @@ export const useTimelineContext = (): ContextProps => {
`);
}
setPipelineRange(pipelineRange);
if (pipelineRange.end_ts) {
// set initial date range to [pipelineEndDate - 7 days, TODAY_DATE]
setDateRange([
DateTime.fromSeconds(pipelineRange.end_ts).minus({ days: 7 }).toISODate(),
TODAY_DATE,
]);
} else {
logWarn('Timeline: no pipeline end date. dateRange will stay null');
setTimelineIsLoading(false);

Check warning on line 162 in www/js/TimelineContext.ts

View check run for this annotation

Codecov / codecov/patch

www/js/TimelineContext.ts#L160-L162

Added lines #L160 - L162 were not covered by tests
}
} catch (e) {
displayError(e, t('errors.while-loading-pipeline-range'));
setTimelineIsLoading(false);
}
}

function loadMoreDays(when: 'past' | 'future', nDays: number) {
const existingRange = queriedDateRange || initialQueryRange;
if (!queriedDateRange) {
logWarn('No queriedDateRange yet - early return from loadMoreDays');
return;

Check warning on line 173 in www/js/TimelineContext.ts

View check run for this annotation

Codecov / codecov/patch

www/js/TimelineContext.ts#L172-L173

Added lines #L172 - L173 were not covered by tests
}
logDebug(`Timeline: loadMoreDays, ${nDays} days into the ${when};
queriedDateRange = ${queriedDateRange}; existingRange = ${existingRange}`);
queriedDateRange = ${queriedDateRange}`);
return loadDateRange(
when == 'past'
? [isoDateWithOffset(existingRange[0], -nDays), existingRange[1]]
: [existingRange[0], isoDateWithOffset(existingRange[1], nDays)],
? [isoDateWithOffset(queriedDateRange[0], -nDays), queriedDateRange[1]]
: [queriedDateRange[0], isoDateWithOffset(queriedDateRange[1], nDays)],

Check warning on line 180 in www/js/TimelineContext.ts

View check run for this annotation

Codecov / codecov/patch

www/js/TimelineContext.ts#L179-L180

Added lines #L179 - L180 were not covered by tests
);
}

Expand All @@ -182,13 +188,12 @@ export const useTimelineContext = (): ContextProps => {
return;
}
const pipelineStartDate = DateTime.fromSeconds(pipelineRange.start_ts).toISODate();
const todayDate = DateTime.now().toISODate();
// clamp range to ensure it is within [pipelineStartDate, todayDate]
// clamp range to ensure it is within [pipelineStartDate, TODAY_DATE]
const clampedDateRange: [string, string] = [
new Date(range[0]) < new Date(pipelineStartDate) ? pipelineStartDate : range[0],
new Date(range[1]) > new Date(todayDate) ? todayDate : range[1],
new Date(range[1]) > new Date(TODAY_DATE) ? TODAY_DATE : range[1],
];
if (clampedDateRange[0] != dateRange[0] || clampedDateRange[1] != dateRange[1]) {
if (clampedDateRange[0] != dateRange?.[0] || clampedDateRange[1] != dateRange?.[1]) {
logDebug('Timeline: loadDateRange setting new date range = ' + clampedDateRange);
setTimelineIsLoading('queued');
setDateRange(clampedDateRange);
Expand Down Expand Up @@ -259,7 +264,7 @@ export const useTimelineContext = (): ContextProps => {
try {
logDebug('timelineContext: refreshTimeline');
setTimelineIsLoading('replace');
setDateRange(initialQueryRange);
setDateRange(null);

Check warning on line 267 in www/js/TimelineContext.ts

View check run for this annotation

Codecov / codecov/patch

www/js/TimelineContext.ts#L267

Added line #L267 was not covered by tests
setQueriedDateRange(null);
setTimelineMap(null);
setRefreshTime(new Date());
Expand Down
2 changes: 1 addition & 1 deletion www/js/diary/list/DateSelect.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ const DateSelect = ({ mode, onChoose, ...rest }: Props) => {
const { colors } = useTheme();
const [open, setOpen] = React.useState(false);
const minMaxDates = useMemo(() => {
if (!pipelineRange) return { startDate: new Date(), endDate: new Date() };
if (!pipelineRange?.start_ts) return { startDate: new Date(), endDate: new Date() };
return {
startDate: new Date(pipelineRange?.start_ts * 1000), // start of pipeline
endDate: new Date(), // today
Expand Down
4 changes: 2 additions & 2 deletions www/js/metrics/MetricsTab.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ const MetricsTab = () => {
const [aggMetricsIsLoading, setAggMetricsIsLoading] = useState(false);

const readyToLoad = useMemo(() => {
if (!appConfig) return false;
if (!appConfig || !dateRange) return false;
const dateRangeDays = isoDatesDifference(...dateRange);
if (dateRangeDays < N_DAYS_TO_LOAD) {
logDebug('MetricsTab: not enough days loaded, trying to load more');
Expand All @@ -132,7 +132,7 @@ const MetricsTab = () => {
}, [readyToLoad, appConfig, timelineIsLoading, timelineMap, timelineLabelMap]);

useEffect(() => {
if (!readyToLoad || !appConfig) return;
if (!readyToLoad || !appConfig || !dateRange) return;
logDebug('MetricsTab: ready to fetch aggMetrics');
setAggMetricsIsLoading(true);
fetchAggMetrics(metricList, dateRange, appConfig).then((response) => {
Expand Down

0 comments on commit 1955618

Please sign in to comment.