-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Orbit banner warning for stale data #523
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
bdbe689
02bd25e
e0acd3f
3b1046f
7e72f1c
d700348
438130c
fbd3c13
08f8515
5bdb7f9
33067e2
765f31b
5c64853
e6aa229
bd201ca
a1aaebe
80ad902
4b88a41
e6b7bfe
1be7dce
2546114
00d210d
1d06684
87221ba
53d2720
3b04eb2
3ae76a0
a62c50d
b837b51
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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> | ||
| : <></>; | ||
| }; |
| 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; | ||
| }); | ||
| }; | ||
|
|
||
| return ( | ||
| <DataWarningsContext.Provider value={[warnings, addWarning, removeWarning]}> | ||
| {children} | ||
| </DataWarningsContext.Provider> | ||
| ); | ||
| }; | ||
|
|
||
| export const useDataWarnings = () => useContext(DataWarningsContext); | ||
| 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, | ||
|
|
@@ -21,6 +29,23 @@ export const useVehicles = (): Vehicle[] | null => { | |
| RawData: VehicleDataMessage, | ||
| defaultResult: null, | ||
| }); | ||
| const checkForStaleData = useEffectEvent(() => { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: I haven't personally delved into
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Although in typing the above and thinking through this again, |
||
| 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
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| [now, mostRecentTimestamp], | ||
| ); | ||
|
|
||
| return result; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,4 +10,5 @@ export type IconName = | |
| | "menu" | ||
| | "red-line" | ||
| | "shield" | ||
| | "sign-out"; | ||
| | "sign-out" | ||
| | "warning-circle"; | ||
| 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(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
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
Setdiff.