From cfb5faa09bb0788e270d100f1f36536a53bcff75 Mon Sep 17 00:00:00 2001 From: Samuel Gunter <29130894+Samathingamajig@users.noreply.github.com> Date: Tue, 6 May 2025 16:07:27 -0500 Subject: [PATCH] fix: course columns on calendar (#587) * fix: course columns on calendar * test: new tests for course cell columns, extract calls to help function * fix: gracefully handle async courses * refactor: split course cell column logic into multiple functions, add comments, and derive fields * chore: comments, for-index to for-of * chore: fix typo * fix: don't use sentry in storybook contexts * fix: silence, brand * refactor: don't rely on calculating columns, find as you go --------- Co-authored-by: doprz <52579214+doprz@users.noreply.github.com> --- .../components/calendar/CalendarGrid.tsx | 42 ++-- src/views/components/calendar/utils.test.ts | 206 +++++++++++++++++- src/views/components/calendar/utils.ts | 134 ++++++++++++ src/views/hooks/useFlattenedCourseSchedule.ts | 1 + 4 files changed, 357 insertions(+), 26 deletions(-) diff --git a/src/views/components/calendar/CalendarGrid.tsx b/src/views/components/calendar/CalendarGrid.tsx index 938e188e..0469ac1a 100644 --- a/src/views/components/calendar/CalendarGrid.tsx +++ b/src/views/components/calendar/CalendarGrid.tsx @@ -2,14 +2,18 @@ import type { Course } from '@shared/types/Course'; import CalendarCourseCell from '@views/components/calendar/CalendarCourseCell'; import Text from '@views/components/common/Text/Text'; import { ColorPickerProvider } from '@views/contexts/ColorPickerContext'; +import { useSentryScope } from '@views/contexts/SentryContext'; import type { CalendarGridCourse } from '@views/hooks/useFlattenedCourseSchedule'; import React, { Fragment } from 'react'; import CalendarCell from './CalendarGridCell'; +import { calculateCourseCellColumns } from './utils'; const daysOfWeek = ['MON', 'TUE', 'WED', 'THU', 'FRI']; const hoursOfDay = Array.from({ length: 14 }, (_, index) => index + 8); +const IS_STORYBOOK = import.meta.env.STORYBOOK; + interface Props { courseCells?: CalendarGridCourse[]; saturdayClass?: boolean; @@ -106,6 +110,12 @@ interface AccountForCourseConflictsProps { // TODO: Possibly refactor to be more concise // TODO: Deal with react strict mode (wacky movements) function AccountForCourseConflicts({ courseCells, setCourse }: AccountForCourseConflictsProps): JSX.Element[] { + // Sentry is not defined in storybook. + // This is a valid use case for a condition hook, since IS_STORYBOOK is determined at build time, + // it doesn't change between renders. + // eslint-disable-next-line react-hooks/rules-of-hooks + const [sentryScope] = IS_STORYBOOK ? [undefined] : useSentryScope(); + // Groups by dayIndex to identify overlaps const days = courseCells.reduce( (acc, cell: CalendarGridCourse) => { @@ -120,31 +130,15 @@ function AccountForCourseConflicts({ courseCells, setCourse }: AccountForCourseC ); // Check for overlaps within each day and adjust gridColumnIndex and totalColumns - Object.values(days).forEach((dayCells: CalendarGridCourse[]) => { - // Sort by start time to ensure proper columnIndex assignment - dayCells.sort((a, b) => a.calendarGridPoint.startIndex - b.calendarGridPoint.startIndex); - - dayCells.forEach((cell, _, arr) => { - let columnIndex = 1; - cell.totalColumns = 1; - // Check for overlaps and adjust columnIndex as needed - for (let otherCell of arr) { - if (otherCell !== cell) { - const isOverlapping = - otherCell.calendarGridPoint.startIndex < cell.calendarGridPoint.endIndex && - otherCell.calendarGridPoint.endIndex > cell.calendarGridPoint.startIndex; - if (isOverlapping) { - // Adjust columnIndex to not overlap with the otherCell - if (otherCell.gridColumnStart && otherCell.gridColumnStart >= columnIndex) { - columnIndex = otherCell.gridColumnStart + 1; - } - cell.totalColumns += 1; - } - } + Object.values(days).forEach((dayCells: CalendarGridCourse[], idx) => { + try { + calculateCourseCellColumns(dayCells); + } catch (error) { + console.error(`Error calculating course cell columns ${idx}`, error); + if (sentryScope) { + sentryScope.captureException(error); } - cell.gridColumnStart = columnIndex; - cell.gridColumnEnd = columnIndex + 1; - }); + } }); return courseCells diff --git a/src/views/components/calendar/utils.test.ts b/src/views/components/calendar/utils.test.ts index 9093aa45..b2d0d41b 100644 --- a/src/views/components/calendar/utils.test.ts +++ b/src/views/components/calendar/utils.test.ts @@ -1,6 +1,7 @@ import { tz } from '@date-fns/tz'; import { Course } from '@shared/types/Course'; import { UserSchedule } from '@shared/types/UserSchedule'; +import type { CalendarGridCourse } from '@views/hooks/useFlattenedCourseSchedule'; import type { Serialized } from 'chrome-extension-toolkit'; import { format as formatDate, parseISO } from 'date-fns'; import { @@ -8,9 +9,17 @@ import { multiMeetingMultiInstructorCourse, multiMeetingMultiInstructorSchedule, } from 'src/stories/injected/mocked'; -import { afterEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { allDatesInRanges, formatToHHMMSS, meetingToIcsString, nextDayInclusive, scheduleToIcsString } from './utils'; +import type { CalendarCourseCellProps } from './CalendarCourseCell'; +import { + allDatesInRanges, + calculateCourseCellColumns, + formatToHHMMSS, + meetingToIcsString, + nextDayInclusive, + scheduleToIcsString, +} from './utils'; // Do all timezone calculations relative to UT's timezone const TIMEZONE = 'America/Chicago'; @@ -477,3 +486,196 @@ describe('scheduleToIcsString', () => { vi.restoreAllMocks(); }); }); + +describe('calculateCourseCellColumns', () => { + let testIdCounter = 0; + + const makeCell = (startIndex: number, endIndex: number): CalendarGridCourse => { + if (endIndex <= startIndex && !(startIndex === -1 && endIndex === -1)) { + throw new Error('Test writer error: startIndex must be strictly less than endIndex'); + } + + const cell = { + calendarGridPoint: { + dayIndex: 1, + startIndex, + endIndex, + }, + componentProps: {} as unknown as CalendarCourseCellProps, + course: {} as unknown as Course, + async: false, + } satisfies CalendarGridCourse; + + /* eslint no-underscore-dangle: ["error", { "allow": ["__test_id"] }] */ + (cell as unknown as { __test_id: number }).__test_id = testIdCounter++; + + return cell; + }; + + /** + * Creates test cases for calculateCourseCellColumns + * @param cellConfigs - Array of [startIndex, endIndex, totalColumns, gridColumnStart, gridColumnEnd] + * @returns Tuple of [cells, expectedCells] + */ + const makeCellsTest = ( + cellConfigs: Array<[number, number, number, number, number]> + ): [CalendarGridCourse[], CalendarGridCourse[]] => { + // Create cells with only start/end indices + const cells = cellConfigs.map(([startIndex, endIndex]) => makeCell(startIndex, endIndex)); + + // Create expected cells with all properties set + const expectedCells = structuredClone(cells); + cellConfigs.forEach((config, index) => { + const [, , totalColumns, gridColumnStart, gridColumnEnd] = config; + expectedCells[index]!.totalColumns = totalColumns; + expectedCells[index]!.gridColumnStart = gridColumnStart; + expectedCells[index]!.gridColumnEnd = gridColumnEnd; + }); + + return [cells, expectedCells]; + }; + + beforeEach(() => { + // Ensure independence between tests + testIdCounter = 0; + }); + + it('should do nothing to an empty array if no courses are present', () => { + const cells: CalendarGridCourse[] = []; + calculateCourseCellColumns(cells); + expect(cells).toEqual([]); + }); + + it('should set the right values for one course cell', () => { + const [cells, expectedCells] = makeCellsTest([[13, 15, 1, 1, 2]]); + + calculateCourseCellColumns(cells); + + expect(cells).toEqual(expectedCells); + }); + + it('should handle two separated courses', () => { + // These two cells can share a column, because they aren't concurrent + const [cells, expectedCells] = makeCellsTest([ + [13, 15, 1, 1, 2], + [16, 18, 1, 1, 2], + ]); + + calculateCourseCellColumns(cells); + + expect(cells).toEqual(expectedCells); + }); + + it('should handle two back-to-back courses', () => { + // These two cells can share a column, because they aren't concurrent + const [cells, expectedCells] = makeCellsTest([ + [13, 15, 1, 1, 2], + [15, 17, 1, 1, 2], + ]); + + calculateCourseCellColumns(cells); + + expect(cells).toEqual(expectedCells); + }); + + it('should handle two concurrent courses', () => { + // These two cells must be in separate columns, because they are concurrent + const [cells, expectedCells] = makeCellsTest([ + [13, 15, 2, 1, 2], + [14, 16, 2, 2, 3], + ]); + + calculateCourseCellColumns(cells); + + expect(cells).toEqual(expectedCells); + }); + + it('should handle a simple grid', () => { + // Two columns + const [cells, expectedCells] = makeCellsTest([ + [13, 15, 2, 1, 2], // start in left-most column + [15, 17, 2, 1, 2], // compact into left column + [13, 17, 2, 2, 3], // take up second column + ]); + + calculateCourseCellColumns(cells); + + expect(cells).toEqual(expectedCells); + }); + + it('should handle a simple grid, flipped', () => { + // Ensures `totalColumns` is calculated correctly + const [cells, expectedCells] = makeCellsTest([ + [13, 17, 2, 1, 2], + [15, 17, 2, 2, 3], + [13, 15, 2, 2, 3], + ]); + + calculateCourseCellColumns(cells); + + expect(cells).toEqual(expectedCells); + }); + + it('should handle a weird grid', () => { + // Three columns + const [cells, expectedCells] = makeCellsTest([ + [13, 15, 3, 1, 2], + [14, 18, 3, 2, 3], + [14, 16, 3, 3, 4], + [15, 17, 3, 1, 2], // compacted into left-most columns + ]); + + calculateCourseCellColumns(cells); + + expect(cells).toEqual(expectedCells); + }); + + it('should handle many clean concurrent courses', () => { + // All cells here are concurrent, 8 columns + const [cells, expectedCells] = makeCellsTest([ + [10, 16, 8, 1, 2], + [12, 16, 8, 2, 3], + [13, 16, 8, 3, 4], + [13, 16, 8, 4, 5], + [13, 19, 8, 5, 6], + [13, 19, 8, 6, 7], + [14, 18, 8, 7, 8], + [15, 19, 8, 8, 9], + ]); + + calculateCourseCellColumns(cells); + + expect(cells).toEqual(expectedCells); + }); + + it('should handle many clean concurrent courses with one partially-concurrent', () => { + // Despite adding another course, we don't need to increase + // the number of columns, because we can compact + const [cells, expectedCells] = makeCellsTest([ + [10, 16, 8, 1, 2], + [11, 15, 8, 2, 3], // new course, only overlaps with some + [12, 16, 8, 3, 4], + [13, 16, 8, 4, 5], + [13, 16, 8, 5, 6], + [13, 19, 8, 6, 7], + [13, 19, 8, 7, 8], + [14, 18, 8, 8, 9], + [15, 19, 8, 2, 3], // compacts to be under new course + ]); + + calculateCourseCellColumns(cells); + + expect(cells).toEqual(expectedCells); + }); + + it("shouldn't crash on courses without times", () => { + const cells = [makeCell(-1, -1), makeCell(-1, -1)]; + cells[1]!.async = true; // see if we can ignore async and non-async courses without times + + const expectedCells = structuredClone(cells); + + calculateCourseCellColumns(cells); + + expect(cells).toEqual(expectedCells); + }); +}); diff --git a/src/views/components/calendar/utils.ts b/src/views/components/calendar/utils.ts index 21fb33bd..7c566136 100644 --- a/src/views/components/calendar/utils.ts +++ b/src/views/components/calendar/utils.ts @@ -6,6 +6,7 @@ import Instructor from '@shared/types/Instructor'; import type { UserSchedule } from '@shared/types/UserSchedule'; import { downloadBlob } from '@shared/util/downloadBlob'; import { englishStringifyList } from '@shared/util/string'; +import type { CalendarGridCourse } from '@views/hooks/useFlattenedCourseSchedule'; import type { Serialized } from 'chrome-extension-toolkit'; import type { DateArg, Day } from 'date-fns'; import { @@ -315,3 +316,136 @@ export const saveCalAsPng = () => { }); }); }; + +/** + * Determines all the connected components in the list of cells, where two cells + * are "connected" if there is a path (potentially through other cells) where + * each neighboring cells have overlapping start/end times + * + * @param cells - An array of cells to go on the calendar grid + * @returns An array of connected components, where the inner array is a list of + * all cells that there's a path to (potentially through other intervals) + * without crossing a time gap + * + * @remarks The internal fields cell.concurrentCells and cell.hasParent are + * modified by this function + * + * @example [[8am, 9am), [8:30am, 10am), [9:30am, 11am)] // is all one connected component + * @example [[8am, 9am), [8:30am, 10am), [10am, 11am)] // has two connected components, [[8am, 9am), [8:30am, 10am)] and [[10am, 11am)]] + */ +const findConnectedComponents = (cells: CalendarGridCourse[]): CalendarGridCourse[][] => { + const connectedComponents: CalendarGridCourse[][] = []; + + for (let i = 0; i < cells.length; i++) { + const cell = cells[i]!; + + if (!cell.concurrentCells || cell.concurrentCells.length === 0) { + // If this cell isn't already part of an existing connected component, + // then we need to make a new one. + connectedComponents.push([]); + } + + connectedComponents.at(-1)!.push(cell); + + for (let j = i + 1; j < cells.length; j++) { + const otherCell = cells[j]!; + if (otherCell.calendarGridPoint.startIndex >= cell.calendarGridPoint.endIndex) { + break; + } + + // By ordering of cells array, we know cell.startTime <= other.startTime + // By the if check above, we know cell.endTime > other.endTime + // So, they're concurrent + // Also, by initializing j to i + 1, we know we don't have duplicates + cell.concurrentCells!.push(otherCell); + otherCell.concurrentCells!.push(cell); + } + } + + return connectedComponents; +}; + +/** + * Assigns column positions to each cell in a set of calendar grid cells. + * Ensures that overlapping cells are placed in different columns. + * + * Inspired by the Greedy Interval-Partitioning algorithm. + * + * @param cells - An array of calendar grid course cells to position, must be + * sorted in increasing order of start time + * @throws Error if there's no available column for a cell (should never happen if totalColumns is calculated correctly) + * @remarks The number of columns created is strictly equal to the minimum needed by a perfectly optimal algorithm. + * The minimum number of columns needed is the maximum number of events that happen concurrently. + * Research Interval Graphs for more info https://en.wikipedia.org/wiki/Interval_graph + */ +const assignColumns = (cells: CalendarGridCourse[]) => { + const availableColumns = [true]; + + for (const cell of cells) { + availableColumns.fill(true); + for (const otherCell of cell.concurrentCells!) { + if (otherCell.gridColumnStart !== undefined) { + availableColumns[otherCell.gridColumnStart - 1] = false; + } + } + + // Find an available column, or create one if all columns are full + let column = availableColumns.indexOf(true); + + if (column === -1) { + column = availableColumns.length; + availableColumns.push(true); + } + + // CSS Grid uses 1-based indexing + cell.gridColumnStart = column + 1; + cell.gridColumnEnd = column + 2; + } + + for (const cell of cells) { + cell.totalColumns = availableColumns.length; + } +}; + +/** + * Calculates the column positions for course cells in a calendar grid. + * This function handles the layout algorithm for displaying overlapping course meetings + * in a calendar view. It identifies connected components of overlapping courses, + * determines the number of columns needed for each component, and assigns appropriate + * column positions to each cell. + * + * @param dayCells - An array of calendar grid course cells for a specific day + */ +export const calculateCourseCellColumns = (dayCells: CalendarGridCourse[]) => { + // Sort by start time, increasing + // This is necessary for the correctness of the column assignment + const cells = dayCells + .filter( + cell => + !cell.async && + cell.calendarGridPoint && + typeof cell.calendarGridPoint.startIndex === 'number' && + cell.calendarGridPoint.startIndex >= 0 + ) + .toSorted((a, b) => a.calendarGridPoint.startIndex - b.calendarGridPoint.startIndex); + + // Initialize metadata + for (const cell of cells) { + cell.concurrentCells = []; + cell.gridColumnStart = undefined; + cell.gridColumnEnd = undefined; + } + + // Construct connected components, set concurrent neighbors + const connectedComponents = findConnectedComponents(cells); + + // Assign columns for each connectedComponents + for (const cc of connectedComponents) { + assignColumns(cc); + } + + // Clean up + for (const cell of cells) { + delete cell.concurrentCells; + } +}; diff --git a/src/views/hooks/useFlattenedCourseSchedule.ts b/src/views/hooks/useFlattenedCourseSchedule.ts index 3d3138a4..24ee7844 100644 --- a/src/views/hooks/useFlattenedCourseSchedule.ts +++ b/src/views/hooks/useFlattenedCourseSchedule.ts @@ -36,6 +36,7 @@ export interface CalendarGridCourse { gridColumnStart?: number; gridColumnEnd?: number; totalColumns?: number; + concurrentCells?: CalendarGridCourse[]; } /**