feat(graphs): add a heat map graph style and default to it - #1186
feat(graphs): add a heat map graph style and default to it#1186JamieRuderman wants to merge 29 commits into
Conversation
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
…ark palette entry
…list bars to daily
…switching to bars
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ea803b397
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42fd120fb3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… a held hover readout
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef768aa0d7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export const timeSeriesFullScale = (type: ITimeSeriesType, resolution: ITimeSeriesResolution): number | undefined => { | ||
| const { unit, scale } = TimeSeriesTypeScale[type] | ||
| if (unit === '%') return scale | ||
| if (unit === 'time') return resolutionSeconds(resolution) |
There was a problem hiding this comment.
For daily duration series in a timezone observing DST, this fixed conversion always returns 86,400 seconds even though a daily API bucket can cover 23 or 25 hours. The newly fixed-scale list graph therefore makes a continuously online device appear partially offline on the spring transition day (and clips a 25-hour bucket in autumn); normalize each daily bucket by its actual local boundary duration or otherwise account for variable-length days.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, but leaving as-is deliberately.
A local calendar day on a spring-forward transition is 23h, so an always-on device accumulates 82,800s against a fixed scale of 86,400 — a bar ~4% short, once a year, in DST zones only.
Two reasons not to chase it:
- Not a regression. With the previous auto-scale the max came from the neighbouring full days (86,400), so the transition day drew at the same 95.8%. In autumn the fixed scale is actually better: auto-scale took its max from the 25h bucket (90,000) and drew every normal day at 96%, where the fixed scale draws them at 100% and clips the long one.
- Heat maps are unaffected. Cells are HOUR buckets, and an hour is 3,600s across a transition — DST removes or repeats whole hours rather than changing their length.
heatmapGridsums the repeated hour into one cell and the d3 scale is.clamp(true).
A correct fix means normalizing each bucket by its own local day duration, which is a per-bucket scale — BarGraph takes a single max, and the value would become a fraction, so the hover readout ("8 minutes, 38 seconds") would need the raw seconds carried alongside. That is a real redesign for a once-a-year 4% artifact that this PR does not introduce.
The sibling finding about keep < 1 is fixed in fbb4c94 — though it was unreachable: I probed the API and it zero-fills its window back past device creation (a device created 2026-06-26 returns all 365 daily buckets to 2025-08-23), so with length + 1 requested there is always more than one bucket.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbb4c946ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aeb49b32ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // set to, so switching style over the same span resolves to the query the | ||
| // list already ran. Refetching would return the data it holds and drop | ||
| // every device back to unloaded on the way through mergeDevice. | ||
| if (listTimeSeriesKey(previous) !== listTimeSeriesKey(options)) return await dispatch.devices.fetchList() |
There was a problem hiding this comment.
When graph settings are changed twice before the first list request completes, each call here starts an unsequenced fetchList, whose response is applied unconditionally by truncateMergeDevices. If the older request returns last, it permanently replaces the current list series with data for the previous options; the small TimeSeries renderer receives no current options and therefore cannot detect the mismatch. Fresh evidence in this revision is that list responses now retain their captured style/type/span metadata, but no list path compares it with the active settings; reject superseded responses or gate them with a request token/current listTimeSeriesKey.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, but pre-existing and out of scope for this PR — filing it separately rather than patching it here.
fetchList has no sequencing of any kind, and it has 25 call sites: search, filter drawer, tag filters, column changes, pagination/load-more, tag CRUD, plans, cloud sync, and the refresh button. SearchField alone re-dispatches it on debounced keystrokes, which races far harder than graph settings can and with more visible consequences. On main, GraphsPage already did await setPersistent(...); await fetchList() for device settings, so two rapid changes raced identically before this PR.
The right fix is a request token on fetchList itself, so every caller benefits. Doing it correctly means handling append (load-more must not drop a page), the total/results writes, and the initialized/appliedName bookkeeping — that is a change to the most heavily used data path in the app and deserves its own PR and its own testing, not a rider on a graphs change.
Gating only the graph dimension here, as suggested, would leave the other 24 callers racing while adding a special case to shared infrastructure — worse than either doing it properly or leaving it alone.
One correction to the finding: the detail view does recover. fetchList sets loaded: false via mergeDevice, so DeviceContextWrapper refetches, and timeSeriesLoading now compares type and span so a mismatched series renders as loading rather than as the wrong graph. The list column is the part that would hold stale bars until the next fetch.
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…off only for the grid
…ad of dropping the current period
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee8e2b1bde
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| onHover={setDisplay} | ||
| /> | ||
| ) : ( | ||
| <BarGraph {...props} data={bars} color={color} height={40} width={200} max={max} min={0} onHover={setDisplay} /> |
There was a problem hiding this comment.
When a bar request is superseded—for example, changing Day to Week or changing the graph type while a full-device fetch is pending—an older response can still arrive last and mark the device loaded. Fresh evidence after the claimed fix is that timeSeriesLoading may detect a type mismatch, but loading is passed only to HeatGraph; this branch renders BarGraph unconditionally, and same-type bar resolution/span mismatches are not detected at all. The stale bars can therefore remain under the current settings indefinitely; discard superseded responses or gate this branch on the complete request identity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining — this is the same fetchList sequencing gap already filed, narrowed to the bar branch.
Traced it: without an out-of-order response there is no persistent staleness here, because listTimeSeriesKey is type-resolution-length and for bars both listTimeSeriesOptions and timeSeriesRequest are identity. So any bar change to type, resolution, or span changes the key and forces a refetch; the only setting that leaves the key untouched is the style toggle, which is exactly the case the fold handles locally. The "indefinitely" scenario needs the unsequenced fetchList described in #1186 (comment).
Gating the bar branch on loading would also make the UX worse rather than better. Bars deliberately keep the previous series on screen during a refetch — the standard stale-while-revalidate behaviour — and a placeholder would flash on every settings change. The heat map needs its placeholder for a different reason: its geometry changes with the span, so drawing the old series would reflow the grid when the new one lands. Bars just redraw in place at the same size.
So the asymmetry is deliberate, not an oversight: loading is passed only where a geometry change has to be absorbed.
…plit the list strip from the details view
1 participant
Adds a Heat map graph style — a day (column) by hour-of-day (row) grid — and makes it the default. Frontend only; the
timeSeriesAPI already supports everything this needs.Why
A device that is online two hours a day and one that is online all day look nearly identical in the current bar graph, because each graph auto-scales to its own peak. Heat cells use an absolute scale instead (100% for percentage types, the bucket's own duration for time types), so devices are directly comparable and a barely-online device reads as pale at a glance.
It also separates two cases the daily bars cannot tell apart. A device with 8% availability that comes up for a scheduled window paints a solid block; one that is flapping paints a uniform haze. Both previously looked like "2 hours".
Changes
GRAPH STYLEsetting (Bar | Heat map) on the Graphs settings page. Picking Heat map scopes Graph unit to Hour and Graph length to days; picking Bar puts them back to Day and the longest span the plan's log limit allows.Whole periods everywhere. The API's window ends with the period still in progress, which is why the last bar in a 30-day chart is always short — it is today so far, not a real dip. Every graph now fetches one period beyond the span it shows (
timeSeriesRequest) and drops it at the data boundary (trimIncompleteinprocessTimeSeries), so bars and heat map columns are always complete. Heat maps drop the whole day in progress rather than its latest bucket; dropping only the bucket leaves a 22-hour stub column in the midnight hour.The device list stays bars, on a fixed y axis. The column has no axis to read a scale off, so auto-scaling to each device's own peak made a device that is barely ever up draw the same as one that is always up. List bars now scale to the absolute ceiling for the bucket — a full day of uptime, or 100%. On an 18px canvas a device at 8% availability averages 1.5px against 17.8px for an always-on one. Event-count types have no ceiling and still scale to themselves.
Friendlier hour labels. The heat map axis reads
midnight / 6am / noon / 6pmwhere a 12 hour clock is the norm, and00:00 / 06:00 / 12:00 / 18:00where it is not. Clock convention is regional rather than a matter of language — en-GB writes 18:00 where en-US writes 6pm — so it comes from the OS locale when that is a region of the app's language, and from the app language otherwise. One locale drives both the choice and the formatting, which keeps a 24 hour language from being forced into a 12 hour shape it has no words for.New default, for devices only. A device is online or it isn't, around the clock, so the hour-of-day grid has something to say about it — devices default to
heatmap / HOUR / 30 days, or the longest the plan allows. Connections are occasional rather than continuous, so most cells in a service heat map are empty and the daily bars read better; services keepbar / DAY. Either can be switched with the new setting. Anyone who has customised their graph settings keeps what they chose.Keeping services on bars also keeps the single-device fetch cheap: a service series is 31 daily points rather than 744 hourly ones, so a device with 20 services costs 1,364 points instead of 15,624.
Payload
The device list is unchanged.
listTimeSeriesOptionscollapses a heat map to DAY resolution before the request is built, and that scoping lives in the query builders rather than in their callers, so a list query cannot accidentally ask for the details view's hourly series.Only the single-device fetch is heavier, which is a deliberate trade.
Verification
midnight 6am noon 6pm, en+en-GB gives00:00 06:00 12:00 18:00, de/es/ja stay 24 hour, and a German app on a US machine falls back to its own convention rather than emitting a mangled label.npm run typecheckandnpm run i18n:checkpass. New strings added to en/ja/de/es.Worth knowing: the API aligns hourly buckets to local hour boundaries, not UTC — a
timezone: "Asia/Kolkata"query returns a window at:30. Row bucketing relies on that.Notes
/simplifyand/code-reviewboth ran over this. The review's findings are fixed except one: devices rehydrated fromlocalForageby an older build carry no style stamp, so they render as bar sparklines until the firstfetchListre-adapts them. Self-correcting, andpersistConfig.versionis version-keyed so an upgrade discards them anyway.lengthis a day count while itsresolutionsets the rows within each day.timeSeriesLengthUnit()centralises that, but a discriminated union onstylewould express it better — deferred because it changes a persisted settings shape.🤖 Generated with Claude Code