Skip to content
Open
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
bdbe689
feat: Initial work to create a context providers for warnings
rycollins Jun 22, 2026
02bd25e
feat: Basic implementation of warning banner for stale data
rycollins Jun 22, 2026
e0acd3f
chore: Unused variables
rycollins Jun 24, 2026
3b1046f
test: Added DataWarningContext provider to text cases
rycollins Jun 25, 2026
7e72f1c
refactor: Move DataWarningsProvider to wrap the elements rather than …
rycollins Jun 25, 2026
d700348
refactor: Switch to using a mocked function rather than wrapping the …
rycollins Jun 25, 2026
438130c
refactor: Cleanup conditional logic for setting stale
rycollins Jun 25, 2026
fbd3c13
test: Banner test cases
rycollins Jun 25, 2026
08f8515
refactor: Change to using array destructuring for state values
rycollins Jun 25, 2026
5bdb7f9
fix: Formatting + removing references to dark/light mode in class names
rycollins Jun 25, 2026
33067e2
refactor: Switch to using a Set for warnings and hide the implementat…
rycollins Jun 25, 2026
765f31b
test: Missed a mock update for app test
rycollins Jun 25, 2026
5c64853
fix: Mutating Set on add warning was causing failure to update
rycollins Jun 25, 2026
e6aa229
fix: Added icon and adjusted styling to address offsets caused by the…
rycollins Jun 26, 2026
bd201ca
fix: Wrapped header, banner and body into a screen height flex box to…
rycollins Jun 29, 2026
a1aaebe
fix: Added a bit of left margin on small viewports when sidebar expanded
rycollins Jun 29, 2026
80ad902
fix: Fixed issue with horizontal scroll on small screen size when sid…
rycollins Jun 29, 2026
4b88a41
fix: Correct sidebar behavior to fill screen when small viewport
rycollins Jun 29, 2026
e6b7bfe
fix: Address sidebar resizing when screen is large enough to accomoda…
rycollins Jun 29, 2026
1be7dce
refactor: Header no longer needs to be sticky
rycollins Jun 29, 2026
2546114
chore: Since header is no longer fixed/sticky, remove z-index
rycollins Jun 29, 2026
00d210d
fix: Eslint
rycollins Jun 29, 2026
1d06684
fix: Switched to useEffectEvent to address a case where the useEffect…
rycollins Jun 30, 2026
87221ba
fix: Wrap header and banner in sticky element (in case that behavior …
rycollins Jun 30, 2026
53d2720
refactor: Moved argument order for dependency array in useChannel to …
rycollins Jun 30, 2026
3b04eb2
fix: Remove no-longer-needed viewport conditional styling when sideba…
rycollins Jun 30, 2026
3ae76a0
fix: Added mostRecentTimestamp to dependency array on stale data check
rycollins Jun 30, 2026
a62c50d
style: Prettier
rycollins Jun 30, 2026
b837b51
fix: Change sidebar default size to exactly match the pre-existing width
rycollins Jul 1, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions js/components/app.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { reload } from "../browser";
import { DataWarningsProvider } from "../contexts/dataWarningsContext";
import { SocketProvider } from "../contexts/socketContext";
import {
ORBIT_BL_FFD,
Expand All @@ -9,6 +10,7 @@ import {
} from "../groups";
import { paths } from "../paths";
import { AppcuesTrackPage } from "./appcues";
import { Banner } from "./banner";
import { Header } from "./header";
import { LadderPage } from "./ladderPage/ladderPage";
import { LandingPage } from "./landingPage";
Expand Down Expand Up @@ -78,11 +80,16 @@ const router = createBrowserRouter([
{
errorElement: <ErrorBoundary />,
element: (
<>
<DataWarningsProvider>
<AppcuesTrackPage />
<Header />
<Outlet />
</>
<div className="flex flex-col h-screen">
<div className="sticky top-0 z-header">
<Banner />
<Header />
</div>
<Outlet />
</div>
</DataWarningsProvider>
),
children: [
{
Expand Down
27 changes: 27 additions & 0 deletions js/components/banner.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { useDataWarnings } from "../contexts/dataWarningsContext";
import { className } from "../util/dom";
import { ReactElement } from "react";

export const Banner = (): ReactElement => {
const [warnings] = useDataWarnings();

return warnings.size > 0 ?
<div className={className(["px-3 py-4 text-xs bg-yellow/25"])}>
<div className="flex flex-row items-center gap-2">
<svg className={className(["h-4 w-4 inline fill-yellow"])}>
<use xlinkHref={"/images/warning-circle.svg"} />
</svg>
<p className="font-semibold uppercase tracking-wide-4 text-slate-800">
Data Issue
</p>
</div>
<div className="ml-5 flex flex-1 flex-col text-slate-600">
<ul className="list-inside list-disc">
{[...warnings].map((warning) => {
return <li key={warning}>Train positions out of date</li>;
})}
</ul>
</div>
</div>
: <></>;
};
2 changes: 1 addition & 1 deletion js/components/header.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ export const Header = () => {
currentLocation.pathname === paths.help;

return (
<div className="w-full bg-gray-200 p-2 flex justify-between fixed h-header z-header">
<div className="w-full bg-gray-200 p-2 flex justify-between h-header">
<Link to={paths.root}>
<img src="/images/logo.svg" alt="MBTA" className="w-44 h-[32px]" />
</Link>
Expand Down
12 changes: 6 additions & 6 deletions js/components/ladderPage/ladderPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,14 @@ export const LadderPage = ({ routeId }: { routeId: RouteId }): ReactElement => {
}, [onEscape]);

return (
<main className="flex h-screen justify-center">
<main className="flex overflow-y-auto overflow-x-hidden justify-center">
{sideBarSelection !== null ?
<SideBar selection={sideBarSelection} close={close} />
: null}
<div
className={className([
"flex overflow-auto transition-all duration-300 ease-in-out w-full",
sideBarSelection && "ml-80 min-[1485px]:ml-0",
"flex transition-all duration-300 ease-in-out overflow-x-auto w-full",
sideBarSelection && "min-[1485px]:ml-30",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue/suggestion: I realized in testing around that ml-30 actually isn't a value supported by tailwind by default, so this actually gets ignored. As spacing gets larger, at a certain point, it only pre-defines classes in certain increments. So either we should pick ml-32 here, or use a custom value in rem or px.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good callout. This conditional styling is actually no longer necessary since the sidebar is no longer out of the document flow; flex box should handle the re-sizing/spacing. Removed.

])}
// Close sidebar when clicking anywhere in the background
onClick={close}
Expand All @@ -59,9 +62,6 @@ export const LadderPage = ({ routeId }: { routeId: RouteId }): ReactElement => {
sideBarSelection={sideBarSelection}
/>
</div>
{sideBarSelection !== null ?
<SideBar selection={sideBarSelection} close={close} />
: null}
</main>
);
};
4 changes: 2 additions & 2 deletions js/components/ladderPage/sidebar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,14 @@ export const SideBar = ({
close: () => void;
}): ReactElement => {
return (
<aside className="fixed flex flex-col left-0 top-[theme(height.header)] w-full sm:w-80 h-[calc(100vh-theme(height.header))] bg-gray-100 transition-transform duration-300 ease-in-out animate-slide-in-from-left">
<aside className="sm:min-w-[350px] sticky flex flex-col left-0 sm:w-80 bg-gray-100 transition-transform duration-300 ease-in-out animate-slide-in-from-left">
<button
className="absolute m-3 top-0 right-0 h-4 w-4 hover:fill-slate-700"
onClick={close}
>
<img src="/images/close.svg" alt="Close" />
</button>
<div className="h-full">
<div className="h-full w-screen sm:w-auto">
<Consist vehicle={selection.vehicle} />
<CurrentTrip vehicle={selection.vehicle} />
<NextTrip vehicle={selection.vehicle} />
Expand Down
43 changes: 43 additions & 0 deletions js/contexts/dataWarningsContext.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import { createContext, ReactNode, useContext, useState } from "react";

export type DataWarning = "vehicle_positions_stale";
export type DataWarnings = Set<DataWarning>;

const DataWarningsContext = createContext<
[DataWarnings, (warning: DataWarning) => void, (warning: DataWarning) => void]
>([
new Set(),
() => {
throw Error("Not implemented");
},
() => {
throw Error("Not implemented");
},
]);
export const DataWarningsProvider = ({ children }: { children: ReactNode }) => {
const [warnings, setWarnings] = useState<DataWarnings>(new Set());
const addWarning: (warning: DataWarning) => void = (warning: DataWarning) => {
setWarnings((warnings) => {
const newSet = new Set(warnings);
newSet.add(warning);
return newSet;
});
};
const removeWarning: (warning: DataWarning) => void = (
warning: DataWarning,
) => {
setWarnings((warnings) => {
const newSet = new Set(warnings);
newSet.delete(warning);
return newSet;
});
};
Comment on lines +19 to +34

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: I like that you broke this out rather than just copying the basic setter that Glides has. Seems nice that each caller doesn't need to do its own Set diff.


return (
<DataWarningsContext.Provider value={[warnings, addWarning, removeWarning]}>
{children}
</DataWarningsContext.Provider>
);
};

export const useDataWarnings = () => useContext(DataWarningsContext);
2 changes: 1 addition & 1 deletion js/hooks/useChannel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,6 @@ export const useChannel = <RawData, Data>({
event,
);
}
}, [topic, event, channel, parser, RawData]);
}, [topic, event, parser, channel, RawData]);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Is there a reason this changed? It's fine either way but if this wasn't intentional maybe drop it from the PR so it doesn't pollute git blame.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, the order swapping was unintentional. Updated

return data;
};
33 changes: 29 additions & 4 deletions js/hooks/useVehicles.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,25 @@
import { useSocket } from "../contexts/socketContext";
import "../models/vehiclePosition";
import { useDataWarnings } from "../contexts/dataWarningsContext";
import { dateTimeFromUnix, useNow } from "../dateTime";
import {
Vehicle,
VehicleDataMessage,
vehicleFromVehicleData,
} from "../models/vehicle";
import { useChannel } from "./useChannel";

const parser = (message: VehicleDataMessage): Vehicle[] => {
return message.data.entities.map((data) => vehicleFromVehicleData(data));
};
import { useCallback, useEffect, useEffectEvent, useState } from "react";

export const useVehicles = (): Vehicle[] | null => {
const now = useNow("minute");
const [, addWarning, removeWarning] = useDataWarnings();
const [mostRecentTimestamp, setMostRecentTimestamp] = useState(
now.toUnixInteger(),
);
const parser = useCallback((message: VehicleDataMessage): Vehicle[] => {
setMostRecentTimestamp(message.data.timestamp);
return message.data.entities.map((data) => vehicleFromVehicleData(data));
}, []);
const socket = useSocket();
const result = useChannel({
socket,
Expand All @@ -21,6 +29,23 @@ export const useVehicles = (): Vehicle[] | null => {
RawData: VehicleDataMessage,
defaultResult: null,
});
const checkForStaleData = useEffectEvent(() => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: I haven't personally delved into useEffectEvent, and I don't think we have examples of it being used elsewhere. Which doesn't mean that you can't/shouldn't use it here, but could you maybe explain the benefits as opposed to just putting this code directly inside the useEffect call?

@rycollins rycollins Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useEffectEvent is new in React 19 so this is also my first time using it.

As I understand it from reading the doc (https://react.dev/reference/react/useEffectEvent), useEffectEvent is for handling firing events from Effects where the functions related to that firing aren't related to the re-rendering logic of the useEffect. In this case addWarnings and setWarnings aren't related to when this useEffect should be re-triggered but would have to be wrapped in a useCallback and included in the dependency array anyway if they weren't separated out into a useEffectEvent.

Although in typing the above and thinking through this again, mostRecentTimestamp should be included in the dependency array so that we don't run into a situation where the stale data banner lingers after data starts flowing again because we're only checking every 1 minute. Will cause more renders which I was trying to avoid but it's probably the correct logic and not including it is kind of an anti-pattern that the useEffectEvent docs advises against. Will update that.

if (now.diff(dateTimeFromUnix(mostRecentTimestamp), "minute").minutes > 3) {
addWarning("vehicle_positions_stale");
} else {
removeWarning("vehicle_positions_stale");
}
});

useEffect(
() => {
checkForStaleData();
},
// Disabling this because it's due to outdated react-hooks lint rules
// Remove when that library is updated
// eslint-disable-next-line react-hooks/exhaustive-deps
Comment on lines +44 to +46

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Interesting. So this is something that is different in later versions of eslint? I see that it otherwise complains that checkForStaleData should be declared as a dependency of this useEffect. Maybe related to the use of useEffectEvent, but can you provide any context on why it shouldn't be a dependency?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has to do with useEffectEvents not being "stable" (i.e. they change on every render) so including it in the dependency array would cause it to re-fire on every render. Apparently this is an intended way of preventing an incorrect use of useEffectEvent. Newer version of ESLint will actually warn if you include it in the dependency array.

There's an explanation of the reasoning behind this here: https://react.dev/reference/react/useEffectEvent#why-are-effect-events-not-stable.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading through this, I think I understand and agree with the use of the effect here. From perusing eslint-plugin-react-hooks I think some of their later releases (possibly 7.1.0) address this? Unclear. In any case I'm happy with this for now until we update.

[now],
);

return result;
};
3 changes: 2 additions & 1 deletion js/icons.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,5 @@ export type IconName =
| "menu"
| "red-line"
| "shield"
| "sign-out";
| "sign-out"
| "warning-circle";
5 changes: 5 additions & 0 deletions js/test/components/app.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ const mockGetMetaContent = getMetaContent as jest.MockedFunction<
typeof getMetaContent
>;

jest.mock("../../contexts/dataWarningsContext", () => ({
__esModule: true,
useDataWarnings: jest.fn(() => [new Set([]), () => {}, () => {}]),
}));

describe("App", () => {
//NOTE: skipping this test while root path temporarily reroutes to /operators.
//TODO: do not skip this once root path is no longer just rerouting.
Expand Down
45 changes: 45 additions & 0 deletions js/test/components/banner.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import { Banner } from "../../components/banner";
import { useDataWarnings } from "../../contexts/dataWarningsContext";
import { render } from "@testing-library/react";
import { MemoryRouter } from "react-router";

jest.mock("../../contexts/dataWarningsContext", () => ({
__esModule: true,
useDataWarnings: jest.fn(),
}));

describe("Landing Page", () => {
beforeEach(() => {
jest.clearAllMocks();
});

test("renders when warning is present", () => {
(useDataWarnings as jest.Mock).mockImplementation(
jest.fn(() => [new Set(["vehicle_positions_stale"]), () => {}, () => {}]),
);

const view = render(
<MemoryRouter>
<Banner />
</MemoryRouter>,
);
expect(view.getByText("Data Issue")).toBeInTheDocument();
expect(view.getByText("Train positions out of date")).toBeInTheDocument();
});

test("is hidden when no warning is present", () => {
(useDataWarnings as jest.Mock).mockImplementation(
jest.fn(() => [new Set(), () => {}, () => {}]),
);

const view = render(
<MemoryRouter>
<Banner />
</MemoryRouter>,
);
expect(view.queryByText("Data Issue")).not.toBeInTheDocument();
expect(
view.queryByText("Train positions out of date"),
).not.toBeInTheDocument();
});
});
3 changes: 2 additions & 1 deletion js/test/hooks/useVehicles.test.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { DataWarningsProvider } from "../../contexts/dataWarningsContext";
import { useChannel } from "../../hooks/useChannel";
import { useVehicles } from "../../hooks/useVehicles";
import { renderHook } from "@testing-library/react";
Expand All @@ -15,7 +16,7 @@ describe("useVehicles", () => {
});
});
test("subscribes to the proper topic", () => {
renderHook(useVehicles);
renderHook(useVehicles, { wrapper: DataWarningsProvider });
expect(mockUseChannel).toHaveBeenCalledWith(
expect.objectContaining({
topic: "vehicles",
Expand Down
1 change: 1 addition & 0 deletions priv/static/images/warning-circle.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.