feat(web,api): day log complete history without layer filter - #112
Conversation
Default all event layers, paginate until total_events, and keep truncated only for signal/gear caps. Display SI via useUnits, quieter drive accent, and guideline-clean page/timeline components. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
One or more custom setup steps configured for this repository failed during this Copilot code review run: Setup steps run before each review. If the review above is missing context, or no review was posted at all, the failing step above may be the cause. See the workflow run for failure details, fix your setup steps configuration, and re-request a review. Note You can configure setup steps for Copilot code review separately from Copilot cloud agent with a |
There was a problem hiding this comment.
🟡 Changes recommended
Critical event-decoding defects and unresolved pagination consistency and completeness risks remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Expands Day Log to load complete history across all layers by default, with paginated retrieval, client-side filtering, virtualization, SI formatting, and updated localization.
Changes:
- Extends backend event assembly, pagination, mappings, and documentation.
- Adds frontend search, category filters, details, timezone handling, and virtualization.
- Updates API types, translations, controls, source displays, and tests.
File summaries
| File | Reviewed change / final findings |
|---|---|
web/src/i18n/en/locale-vehicles.json |
Updates generated vehicle translations. |
web/src/i18n/en.json |
Adds Day Log translations. |
web/src/features/vehicles/pages/DayLogPage.tsx |
Wires the complete-history page and removes layer URL filters. |
web/src/features/vehicles/pages/DayLogPage.test.tsx |
Tests all-layer rendering and filtering. |
web/src/features/vehicles/lib/daylog.ts |
Adds categories and display helpers. Moderate (1 vote): advertised time search cannot match event timestamps. |
web/src/features/vehicles/lib/daylog.test.ts |
Tests Day Log helpers. |
web/src/features/vehicles/components/daylog/DayLogTimeline.tsx |
Adds filtering, search, virtualization, and details. Moderate (2 votes): time search omits timestamps and timezone from memo dependencies. |
web/src/features/vehicles/components/daylog/DayLogSources.tsx |
Displays source completeness information. |
web/src/features/vehicles/components/daylog/DayLogControls.tsx |
Maintains date and vehicle controls. |
web/src/api/types.ts |
Defines complete-history response fields. Nit (1 vote): DayLogLayer documentation still describes layers as off by default. |
web/src/api/hooks/useDayLog.ts |
Fetches all event pages. Critical (2 votes): offset pagination can duplicate or omit events as data changes. Moderate (2 votes): empty pages can silently produce an incomplete successful result. |
web/src/api/hooks/useDayLog.test.ts |
Tests pagination behavior. |
internal/database/daylog/repo.go |
Adds Day Log queries and typed signal reads. Moderate (1 vote): the baseline query excludes unknown security event types and can lose prior transition context. |
internal/database/daylog/repo_test.go |
Tests repository query shapes. |
internal/api/daylog/handler.go |
Handles complete-history pagination and layer defaults. Nit (2 votes): the handler lacks the API tracing span and propagated instrumentation. |
internal/api/daylog/handler_test.go |
Tests layer and pagination contracts. |
internal/api/daylog/events.go |
Builds, sorts, and pages timeline events. Critical (2 votes): HVAC values read from IntValue although production stores booleans. Critical (1 vote): enum mappings read only IntValue while production values use StrValue. Moderate (1 vote): pagination occurs after full fan-out, merge, and sort for every page. |
internal/api/daylog/events_test.go |
Tests event assembly and paging. |
internal/api/daylog/doc.go |
Documents the API taxonomy and pagination. |
Review details
Suppressed comments (4)
internal/api/daylog/events.go:437
limit/offsetare applied only after all source rows have been queried, edge-detected, merged, and sorted. Since the frontend now makes one request per page, a large day repeats the full database fan-out and sort for every page (up to the signal/gear caps), so this pagination does not bound server work and can hit the 15-second handler timeout. Page or cursor source reads, or cache one stable assembled snapshot for the sequence.
total := len(events)
limit := in.Limit
if limit <= 0 {
limit = dayLogDefaultLimit
}
internal/database/daylog/repo.go:319
SecurityEventsintentionally returns unknownevent_typerows, but this baseline query only loads the three currently known types. For an unknown/generic security row whose previous state exists before the window,BuildTimelinestartsfromat nil, so the new all-security-events behavior still loses the transition context. Select the latest prior row for every event type rather than applying this allowlist, and update the query arguments accordingly.
rows, err := r.pool.Query(ctx, dayLogSecurityPrevSQL, vehicleID, windowStart, []string{"locked", "sentry_mode", "valet_mode_enabled"})
web/src/api/types.ts:3664
- The response now documents omitted
layersas “all layers,” but theDayLogLayercomment above still says layers are off by default. Update that stale alias documentation so callers do not infer the old filtered-by-default behavior.
/** Requested optional layers (echo). Omitted upstream means all layers. */
layers: string[]
web/src/features/vehicles/lib/daylog.ts:243
- The UI placeholder promises time search, but this keyword set never includes
event.ts(or the formatted clock time), so entering a visible timestamp such as15:04:05cannot match any row. Add the event timestamp in the same representation users see, or remove “time” from the placeholder.
export function eventKeywords(event: DayLogEvent): string {
const parts: string[] = [event.type, event.source, event.layer, categoryOf(event.type)];
const payload = event.payload ?? {};
for (const key of ['door', 'window', 'component', 'field', 'event_type', 'from', 'to', 'gear', 'version', 'status', 'start_place', 'end_place']) {
const v = payload[key];
if (typeof v === 'string' && v !== '') parts.push(v);
else if (typeof v === 'number' || typeof v === 'boolean') parts.push(String(v));
}
return parts.join(' ').toLowerCase();
- Files reviewed: 19/19 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| fromShort, fromOK := enumShort(turnSignalShort, edge.From.IntValue) | ||
| toShort, toOK := enumShort(turnSignalShort, edge.To.IntValue) | ||
| // The direction component comes from the NEW state; the | ||
| // previous state rides in from/to for the transition line. | ||
| if toOK { | ||
| e.Payload["component"] = toShort | ||
| } | ||
| e.Payload["from"] = enumDisplay(fromShort, fromOK, edge.From.IntValue) | ||
| e.Payload["to"] = enumDisplay(toShort, toOK, edge.To.IntValue) | ||
| e.Payload["from_value"] = typedSignalValue(edge.From) | ||
| e.Payload["to_value"] = typedSignalValue(edge.To) |
| toShort, toOK := enumShort(hvacShort, edge.To.IntValue) | ||
| fromShort, fromOK := enumShort(hvacShort, edge.From.IntValue) |
| while (events.length < total) { | ||
| const params = new URLSearchParams(base); | ||
| params.set('offset', String(offset)); | ||
| const page = await request<DayLogResponse>(`/day-log?${params.toString()}`, { signal }); |
| const got = page.events ?? []; | ||
| if (got.length === 0) break; |
| const title = t(`dayLog.events.${e.type}`, e.type).toLowerCase(); | ||
| return title.includes(query) || eventKeywords(e).includes(query); | ||
| }); | ||
| }, [all, hidden, query, t]); |
| // means every layer: the default view is the complete history. | ||
| // Returns 200 with an empty events array for a quiet day, 404 for an | ||
| // unknown vehicle. | ||
| func (h *Handler) Get(w http.ResponseWriter, r *http.Request) { |
The history-fail case can paint the playback heading before both QueryError copies land. Wait for two Can't reach server strings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Event-state reconstruction defects and inefficient, inconsistent pagination remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (14)
Previously missed (2) — in code that hasn't changed since the last review.
internal/api/daylog/handler.go:300
- These limit/offset values are applied only after the handler has queried every source and
BuildTimelinehas assembled and sorted the entire day. The frontend now requests one page at a time untiltotal_events, so a large day causes the same DB fan-out, signal-cap scan, event construction, and sort to be repeated for every page; move pagination into the read model or introduce a server-side snapshot/cursor so pages reuse one assembly.
internal/database/daylog/repo.go:319 - The builder deliberately emits unrecognized security event types as generic events, and the security schema documents types beyond these three (for example
airbag_deployedandcrash_state). Restricting the pre-window baseline query to onlylocked,sentry_mode, andvalet_mode_enabledmeans a generic event that is the first row of the selected day loses an availablefromstate. Seed the baseline for every event type (or use the same complete event-type registry) and add a regression case for a generic type.
internal/api/daylog/doc.go:68
- This new contract says
signal_logstores proto enum numbers and that the timeline's numeric maps are the source of truth, but the touched production fields do not follow that shape: turn/window values are persisted instr_valueandHvacPowerinbool_value. This documentation both misleads API consumers and explains why the current reader misses real transitions; document the field-specific canonical types instead.
// signal_log stores proto enum NUMBERS. The timeline labels them with
// read-side display maps (turnSignalShort, windowShort, hvacShort,
// gearShortForm) that cite their generated sources
// (internal/tesla/protomodel/enum_parsers_gen.go + the proto). These
// maps do not import proto bindings and do not re-run the codec's
// prefix-trim; the parity test asserts every entry against the
// generated String() output. Unknown numbers render raw, never guessed.
internal/api/daylog/events.go:750
HvacPoweris canonicalized to a Go bool and persisted insignal_log.bool_value, so real rows have nilIntValuehere. This makes every HVAC transition fall through tohvac_offand drops itsfrom/tostates; useBoolValue(toBool/setBoolFromTo) instead of the enum map, and update the fixtures to use bool rows.
case "HvacPower":
{
toShort, toOK := enumShort(hvacShort, edge.To.IntValue)
fromShort, fromOK := enumShort(hvacShort, edge.From.IntValue)
internal/api/daylog/events.go:720
- Fleet enum values are canonical short strings in
signal_log.str_value, but this mapping only readsIntValue. Consequently real turn-signal transitions have no component and nilfrom/tolabels instead of rendering Left/Right/Off; readStrValue(while preserving an unknown raw fallback) here.
case "LightsTurnSignal":
{
e := mk("turn_signal", LayerTurnSignals)
fromShort, fromOK := enumShort(turnSignalShort, edge.From.IntValue)
toShort, toOK := enumShort(turnSignalShort, edge.To.IntValue)
// The direction component comes from the NEW state; the
// previous state rides in from/to for the transition line.
if toOK {
e.Payload["component"] = toShort
}
e.Payload["from"] = enumDisplay(fromShort, fromOK, edge.From.IntValue)
e.Payload["to"] = enumDisplay(toShort, toOK, edge.To.IntValue)
internal/api/daylog/events.go:375
- This new branch maps
valet_mode_enabledtovalet_on/valet_off, but the package contract still says ValetMode transitions are out of v1 scope and intentionally not mapped (internal/api/daylog/doc.go:85-86). Update the API taxonomy documentation to describe these emitted event types and payloads, or remove the mapping so the documented contract and response behavior agree.
case "valet_mode_enabled":
typ := "valet_unknown"
if s.ToState != nil {
switch *s.ToState {
case "true":
internal/api/daylog/events.go:874
gearShortFormexplicitly supports both full proto tokens and short tokens, but this comparison happens before normalization. A storedShiftStateDfollowed byDis the same gear state yet produces a false gear event; compare the normalized forms and keep the raw values only for payload provenance.
if t.Gear != prev.Gear {
out = append(out, gearEdge{From: prev, To: t})
internal/api/daylog/handler.go:255
- This expanded handler performs the full day-log fan-out, but it still has no
otel.Tracer("api")span, span error recording, or trace ID in its error logs. That breaks the established API observability pattern used by handlers such asinternal/api/drives/listing.go:21-47, making slow page requests and source failures hard to correlate; add the span at entry and record each early-return error.
signalRows, err := h.repo.SignalRows(ctx, p.vehicleID, SignalFieldsForLayers(p.layers), p.start, p.end, dayLogSignalRowCap+1)
if err != nil {
log.Error().Err(err).Int64("vehicle_id", p.vehicleID).Msg("daylog: signal query failed")
httpx.WriteError(w, http.StatusInternalServerError, "failed to load signal edges")
return
web/src/api/hooks/useDayLog.ts:56
- When a subsequent page is empty before
total_events, thisbreakreturns a successful result containing only a partial history while leavingtruncatedfalse. The UI cannot distinguish that from a complete day, so surface a query error (or an explicit incomplete flag) for a stalled/inconsistent page instead of silently accepting missing events.
if (got.length === 0) break;
web/src/api/hooks/useDayLog.ts:54
- Each loop iteration issues another request, but the handler reruns the complete source fan-out and rebuilds/sorts the entire timeline before applying this offset. A multi-page day therefore repeats the same database work and O(E log E) merge for every page, leading to minutes of sequential loading and potentially exhausting the route rate limit; page a stable server-side snapshot/cursor or assemble once and page that result.
while (events.length < total) {
const params = new URLSearchParams(base);
params.set('offset', String(offset));
const page = await request<DayLogResponse>(`/day-log?${params.toString()}`, { signal });
web/src/api/hooks/useDayLog.ts:54
- The all-pages loop uses offsets against a dataset that can change while today’s telemetry is being ingested. If a new event is inserted before the next offset, the next response can skip an existing event (or duplicate one), yet the loop still stops when it reaches the earlier
total_events; use a stable server-side snapshot/cursor or another consistency mechanism for the complete-history fetch.
let offset = events.length;
while (events.length < total) {
const params = new URLSearchParams(base);
params.set('offset', String(offset));
const page = await request<DayLogResponse>(`/day-log?${params.toString()}`, { signal });
web/src/features/vehicles/components/daylog/DayLogTimeline.tsx:76
- The placeholder advertises time search, but
eventKeywordscontains no timestamp and the predicate only checks the localized title plus metadata/payload. Searching for a visible timestamp such as08:04:05therefore returns no match; include the event's localized display time in this predicate.
return title.includes(query) || eventKeywords(e).includes(query);
web/src/features/vehicles/components/daylog/DayLogTimeline.tsx:99
- Adding the virtualized list here leaves
DayLogTimeline.tsxat 546 lines while it still combines filter controls, query states, virtualization, row rendering, expanded payload details, and a large event-formatting switch. The frontend component guideline caps component files at 200 lines; extract the row/details anddescribeEventlogic into focused files so the list remains maintainable and independently testable.
const virtualizer = useVirtualizer({
count: rows.length,
getScrollElement: () => parentRef.current,
estimateSize: () => 64,
overscan: 12,
web/src/features/vehicles/components/daylog/DayLogTimeline.tsx:153
- When the capped signal/gear inputs contain only baselines or unchanged samples,
BuildTimelinecan return no events whiletruncatedis true. This branch renders only the empty state; the truncation warning below the non-empty branch is unreachable, so the user is told nothing was recorded without being told records were omitted. Render the cap warning in the empty branch or outside theall.lengthconditional.
) : all.length === 0 ? (
<EmptyState
icon={<Icons.moon className="h-8 w-8" />}
message={t('dayLog.timeline.empty', 'Nothing recorded this day.')}
description={t(
- Files reviewed: 20/20 changed files
- Comments generated: 1
- Review effort level: Lite
| if window, ok := windowFieldSuffix[edge.Field]; ok { | ||
| e := mk("window", LayerDoorsWindow) | ||
| e.Payload["window"] = window | ||
| e.Payload["value"] = typedSignalValue(edge.Row) | ||
| fromShort, fromOK := enumShort(windowShort, edge.From.IntValue) | ||
| toShort, toOK := enumShort(windowShort, edge.To.IntValue) | ||
| e.Payload["from"] = enumDisplay(fromShort, fromOK, edge.From.IntValue) | ||
| e.Payload["to"] = enumDisplay(toShort, toOK, edge.To.IntValue) | ||
| e.Payload["from_value"] = typedSignalValue(edge.From) | ||
| e.Payload["to_value"] = typedSignalValue(edge.To) |
|
One or more custom setup steps configured for this repository failed during this Copilot code review run: Setup steps run before each review. If the review above is missing context, or no review was posted at all, the failing step above may be the cause. See the workflow run for failure details, fix your setup steps configuration, and re-request a review. Note You can configure setup steps for Copilot code review separately from Copilot cloud agent with a |
Summary
total_events;truncatedis only for signal/gear caps.useUnits; drive accent uses quieter cyan#0891b2.DayLogLayerimport, TS fragment, timezone,Texttitles, test virtualizer mock.Test plan
go test ./internal/api/daylog/ ./internal/database/daylog/npx tsc --noEmit