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>

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[]) => {
+    Object.values(days).forEach((dayCells: CalendarGridCourse[], idx) => {
-        // Sort by start time to ensure proper columnIndex assignment
+        try {
-        dayCells.sort((a, b) => a.calendarGridPoint.startIndex - b.calendarGridPoint.startIndex);
+            calculateCourseCellColumns(dayCells);
-
+        } catch (error) {
-        dayCells.forEach((cell, _, arr) => {
+            console.error(`Error calculating course cell columns ${idx}`, error);
-            let columnIndex = 1;
+            if (sentryScope) {
-            cell.totalColumns = 1;
+                sentryScope.captureException(error);
-            // 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;
-                    }
-                }
             }
-            cell.gridColumnStart = columnIndex;
+        }
-            cell.gridColumnEnd = columnIndex + 1;
-        });
     });
 
     return courseCells

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<CalendarGridCourse[]>(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<CalendarGridCourse[]>(cells);
+
+        calculateCourseCellColumns(cells);
+
+        expect(cells).toEqual(expectedCells);
+    });
+});

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;
+    }
+};

src/views/hooks/useFlattenedCourseSchedule.ts

@@ -36,6 +36,7 @@ export interface CalendarGridCourse {
     gridColumnStart?: number;
     gridColumnEnd?: number;
     totalColumns?: number;
+    concurrentCells?: CalendarGridCourse[];
 }
 
 /**