feat: improve logs view with keyset pagination, column selection, and sticky chart

[trieloff]

Summary

Improves the logs view across three dimensions:

• Keyset pagination (Switch logs paging to keyset pagination #84): Replaces OFFSET-based pagination with cursor-based keyset pagination using timestamp < cursor. Eliminates O(n) performance degradation on deep pages and prevents duplicate/missing rows when new data arrives during paging.
• Visible column selection (Reduce logs payload by selecting visible columns #87): Replaces SELECT * with an explicit column list built from LOG_COLUMN_ORDER. The detail modal fetches the full row on demand via a new log-detail.sql query, keeping payloads small while preserving all fields when inspecting individual entries.
• Sticky collapsible chart ([Feature][Logs]: Keep the chart pinned to the top while scrolling through logs and make it collapsible #66): Pins the chart to the viewport top when scrolling through logs. Adds a collapse/expand toggle with localStorage persistence. Synchronizes the chart scrubber line with the topmost visible log row's timestamp, and scrolling to the matching time position on chart hover.

Closes #84, closes #87, closes #66

Testing Done

• All 495 tests pass (npm test)
• Lint passes (npm run lint)
• New tests added for:
  • PaginationState cursor tracking (js/pagination.test.js)
  • Column list generation (js/columns.test.js)
• Verified combined integration of all three features

Checklist

• Tests pass (npm test)
• Lint passes (npm run lint)
• Documentation updated (if applicable)

[github-actions]

Preview deployment

Preview is live at: https://klickhaus.aemstatus.net/preview/pr-122/dashboard.html

Updated for commit 9c8ac05

[Copilot]

Pull request overview

This PR implements three interconnected improvements to the logs view: keyset pagination for better performance on deep pages, selective column loading to reduce payload size, and a sticky collapsible chart with bidirectional scroll synchronization.

Changes:

• Replaced OFFSET-based pagination with cursor-based keyset pagination using the last row's timestamp, eliminating O(n) performance degradation and preventing duplicate/missing rows during paging
• Introduced selective column loading via buildLogColumnsSql(), fetching only visible columns for the table view and loading full row data on-demand when opening the detail modal
• Added sticky positioning for the chart section when logs view is active, with a collapsible toggle that persists state to localStorage and bidirectional synchronization between chart hover and log scroll position

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
sql/queries/logs.sql	Replaced SELECT * with {{columns}} placeholder for selective column loading
sql/queries/logs-more.sql	Added cursor-based WHERE clause (timestamp < toDateTime64('{{cursor}}', 3)) and removed OFFSET for keyset pagination
sql/queries/log-detail.sql	New query to fetch complete row data by timestamp and host for detail modal
js/sql-loader.js	Registered new log-detail SQL template
js/pagination.js	Replaced offset tracking with cursor tracking, extracting timestamp from last row
js/pagination.test.js	Updated all tests to use cursor-based pagination with row objects instead of count
js/columns.js	Added buildLogColumnsSql() to generate comma-separated, backtick-quoted column list
js/columns.test.js	New test file covering column list generation, deduplication, and ordering
js/logs.js	Refactored detail modal to async fetch full row, added chart collapse toggle with localStorage persistence, and implemented bidirectional scroll sync between chart and logs
js/chart.js	Added setScrubberPosition() and setOnChartHoverTimestamp() for scroll synchronization
js/dashboard-init.js	Wired up chart hover callback with throttling to trigger log scrolling
dashboard.html	Added collapse toggle button to chart section
css/chart.css	Added sticky positioning, collapse toggle styling, and collapsed state transitions
css/modals.css	Added loading state styling for detail modal

Comments suppressed due to low confidence (1)

css/chart.css:60

• The mobile media query overrides position: relative for .chart-section, which will break the sticky positioning on mobile devices when logs view is active. This is because the .logs-active .chart-section rule (line 13-17) sets position: sticky, but the mobile media query (line 56-60) will override it back to position: relative.

The mobile media query should preserve sticky positioning for logs view. Update the media query to:

@media (max-width: 600px) {
  .chart-section {
    margin: 0 -12px 16px -12px;
  }
  .chart-section:not(.logs-active) {
    position: relative;
  }
}

@media (max-width: 600px) {
  .chart-section {
    margin: 0 -12px 16px -12px;
    position: relative;
  }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The cursor value is extracted directly from row data (rows[resultLength - 1].timestamp) and interpolated into SQL without validation or sanitization. While the timestamp comes from the database initially, it's stored in client-side state and could potentially be manipulated.

The cursor should be validated to ensure it matches a timestamp format before being interpolated into SQL, or use parameterized queries if supported by the query layer. At minimum, add validation in fetchFullRow and loadMoreLogs to ensure the timestamp value is in the expected format (e.g., matches a timestamp regex) before passing to loadSql.

Suggested change

	WHERE {{timeFilter}} AND timestamp < toDateTime64('{{cursor}}', 3) {{hostFilter}} {{facetFilters}} {{additionalWhereClause}}
	WHERE {{timeFilter}} AND timestamp < toDateTime64(replaceAll('{{cursor}}', '''', ''), 3) {{hostFilter}} {{facetFilters}} {{additionalWhereClause}}

[Copilot]

The timestamp and host values are extracted from row data and interpolated into SQL. While host is escaped using .replace(/'/g, "\\'"), the timestamp is passed through String(timestamp) without validation or escaping.

This creates a potential SQL injection vector if the timestamp value has been tampered with in client-side state. Add validation to ensure timestamp matches the expected format (YYYY-MM-DD HH:MM:SS.mmm) before interpolation, or apply the same escaping pattern used for host.

Suggested change

	WHERE timestamp = toDateTime64('{{timestamp}}', 3) AND `request.host` = '{{host}}'
	WHERE match('{{timestamp}}', '^[0-9]{4}-[0-9]{2}-[0-9]{2} [0-9]{2}:[0-9]{2}:[0-9]{2}\\.[0-9]{3}$')
	AND timestamp = toDateTime64('{{timestamp}}', 3)
	AND `request.host` = '{{host}}'

[Copilot]

This operation is inefficient for finding the closest timestamp. For a large dataset with thousands of rows, this performs a linear O(n) scan on every chart hover event.

Consider these optimizations:

1. Since state.logsData is sorted by timestamp DESC (from the SQL ORDER BY), use binary search to find the closest timestamp in O(log n) time
2. Store logs in a more efficient data structure for timestamp lookups (e.g., a Map indexed by rounded timestamps)
3. Add early exit conditions if the difference starts increasing (only works if data is sorted)

Suggested change

	let closestIdx = 0;
	let closestDiff = Infinity;
	for (let i = 0; i < state.logsData.length; i += 1) {
	const row = state.logsData[i];
	if (!row.timestamp) {
	closestIdx += 0; // eslint: no-continue workaround - skip rows without timestamp
	} else {
	const rowMs = parseUTC(row.timestamp).getTime();
	const diff = Math.abs(rowMs - targetMs);
	if (diff < closestDiff) {
	closestDiff = diff;
	closestIdx = i;
	}
	}
	}
	// Helper to get the nearest index with a valid timestamp starting from `start`
	function findNearestValidIndex(start, lowBound, highBound) {
	let left = start;
	let right = start;
	while (left >= lowBound || right <= highBound) {
	if (left >= lowBound) {
	const leftRow = state.logsData[left];
	if (leftRow && leftRow.timestamp) return left;
	left -= 1;
	}
	if (right <= highBound) {
	const rightRow = state.logsData[right];
	if (rightRow && rightRow.timestamp) return right;
	right += 1;
	}
	}
	return -1;
	}
	const n = state.logsData.length;
	let low = 0;
	let high = n - 1;
	let candidateIdx = -1;
	// Binary search on timestamps sorted DESC
	while (low <= high) {
	const mid = Math.floor((low + high) / 2);
	const validMid = findNearestValidIndex(mid, low, high);
	if (validMid === -1) {
	// No valid timestamps in this range
	break;
	}
	const row = state.logsData[validMid];
	const rowMs = parseUTC(row.timestamp).getTime();
	candidateIdx = validMid;
	if (rowMs === targetMs) {
	break;
	}
	// Data is sorted by timestamp DESC (newest first).
	if (rowMs < targetMs) {
	// Target is newer (larger ms) than this row, so move left.
	high = validMid - 1;
	} else {
	// Target is older (smaller ms) than this row, so move right.
	low = validMid + 1;
	}
	}
	// Determine the closest index among neighbors around the insertion point.
	const candidates = [];
	if (candidateIdx !== -1) {
	candidates.push(candidateIdx);
	}
	const neighborLow = findNearestValidIndex(low, 0, n - 1);
	if (neighborLow !== -1) {
	candidates.push(neighborLow);
	}
	const neighborHigh = findNearestValidIndex(high, 0, n - 1);
	if (neighborHigh !== -1) {
	candidates.push(neighborHigh);
	}
	if (candidates.length === 0) {
	// Fallback: no rows with timestamps, nothing to scroll to.
	return;
	}
	let closestIdx = candidates[0];
	let closestDiff = Math.abs(
	parseUTC(state.logsData[closestIdx].timestamp).getTime() - targetMs,
	);
	for (let i = 1; i < candidates.length; i += 1) {
	const idx = candidates[i];
	const row = state.logsData[idx];
	if (!row || !row.timestamp) continue;
	const rowMs = parseUTC(row.timestamp).getTime();
	const diff = Math.abs(rowMs - targetMs);
	if (diff < closestDiff) {
	closestDiff = diff;
	closestIdx = idx;
	}
	}

[Copilot]

The throttle implementation has a subtle timing issue. When remaining > 0 and a timer is set, the function will be called after remaining milliseconds with the arguments from the FIRST deferred call, not the MOST RECENT call. This means rapid calls will execute with stale arguments.

This is particularly problematic for scroll/hover handlers where the most recent position is what matters. Consider storing and using the most recent arguments:

function throttle(fn, delay) {
  let lastCall = 0;
  let timer = null;
  let pendingArgs = null;
  return (...args) => {
    pendingArgs = args;
    const now = Date.now();
    const remaining = delay - (now - lastCall);
    if (remaining <= 0) {
      if (timer) {
        clearTimeout(timer);
        timer = null;
      }
      lastCall = now;
      pendingArgs = null;
      fn(...args);
    } else if (!timer) {
      timer = setTimeout(() => {
        lastCall = Date.now();
        timer = null;
        fn(...pendingArgs);
        pendingArgs = null;
      }, remaining);
    }
  };
}

[Copilot]

The new scroll synchronization features (scrollLogsToTimestamp, syncScrubberToScroll, throttle) and chart collapse functionality (initChartCollapseToggle, updateCollapseToggleLabel) lack test coverage. While integration tests verify the combined functionality, these complex interactive features would benefit from unit tests.

Consider adding tests for:

1. scrollLogsToTimestamp - finding closest timestamp with various edge cases (empty data, single row, sorted vs unsorted)
2. throttle - ensuring proper timing behavior and argument handling
3. Chart collapse state persistence and restoration from localStorage
4. Edge cases in scroll sync (no chart section, no visible rows, etc.)

[Copilot]

When the page first loads or when switching to logs view, the collapse toggle button's label is not initialized. It will show the default text from the HTML ("Hide chart") regardless of the actual collapsed state restored from localStorage.

Add a call to updateCollapseToggleLabel() at the end of initChartCollapseToggle() to ensure the button label matches the initial state.

Suggested change

	});
	});
	updateCollapseToggleLabel();

[Copilot]

The chart hover callback triggers on every mousemove event within the chart, which fires very frequently (potentially 60+ times per second). While there's a 300ms throttle in dashboard-init.js, the callback is still invoked on every mousemove, and the throttle only prevents the scroll action itself.

This could cause performance issues because:

1. getTimeAtX(x) is called on every mousemove regardless of throttling
2. The callback function is invoked on every mousemove
3. Multiple layers of function calls happen before the throttle takes effect

Consider moving the throttling logic into the chart.js mousemove handler itself, or only calling onChartHoverTimestamp when the timestamp actually changes significantly (e.g., by more than 1 second).

[Copilot]

The collapse toggle button uses HTML entities (▲ and ▼) for up/down arrows combined with text. Screen readers will announce these as "Black Up-pointing Triangle Hide chart" which may be confusing since the visual arrow is redundant with the text.

Consider using either:

1. Pure text with CSS icons/pseudo-elements for the visual arrows
2. An aria-label that describes the action more clearly: aria-label="Hide chart" or aria-label="Show chart"
3. Hiding the arrow from screen readers: <span aria-hidden="true">&#9650;</span> Hide chart

This ensures a better experience for users relying on assistive technology.

Suggested change

	<button class="chart-collapse-toggle" id="chartCollapseToggle" title="Collapse chart">&#9650; Hide chart</button>
	<button class="chart-collapse-toggle" id="chartCollapseToggle" title="Collapse chart"><span aria-hidden="true">&#9650;</span> Hide chart</button>

[Copilot]

Column names from pinnedColumns are directly interpolated into SQL with only backtick quoting, without validation. If pinnedColumns contains malicious values (e.g., from tampered client state), this could lead to SQL injection.

While pinned columns are likely controlled by the application, they should be validated against a whitelist of allowed column names before being used in SQL queries. Consider adding validation in buildLogColumnsSql to ensure all column names match expected patterns or exist in a known column definition list.

[Copilot]

When pagination.cursor is null (which happens on first load before any page is recorded), this will interpolate as the string "null" in the SQL query: timestamp < toDateTime64('null', 3). This will cause a SQL error.

The logs-more query is intended only for subsequent pages, not the initial load. However, there's no guard preventing loadMoreLogs() from being called when cursor is still null. Consider one of these solutions:

1. Add a check in loadMoreLogs() to ensure cursor is not null before proceeding
2. Modify the SQL template to handle null cursor (e.g., using a conditional or a very large default date)
3. Ensure canLoadMore() returns false when cursor is null

Suggested change

	WHERE {{timeFilter}} AND timestamp < toDateTime64('{{cursor}}', 3) {{hostFilter}} {{facetFilters}} {{additionalWhereClause}}
	WHERE {{timeFilter}} AND CASE WHEN '{{cursor}}' = 'null' OR '{{cursor}}' = '' THEN 1 ELSE timestamp < toDateTime64('{{cursor}}', 3) END {{hostFilter}} {{facetFilters}} {{additionalWhereClause}}

[trieloff]

Usability issues after spot-checking

• when the logs view has been opened, loading logs must be prioritized and all non-visible facet/breakdown/investigation requests should be canceled and resumed only when no log requests are pending
• the ghost rows are styled inconsistently, the "loading" indicator makes them blow up in height, and there is no label (the best label would be something like "and 1334234 more log entries"
• the ghost rows in the middle of the data, I can't see them, not sure if they have been built
• overall it still feels very much work in progress