Code Review

This PR fixes a solid cluster of real bugs (dedup, generation guard, column preservation, cursor parsing, aggregate cleanup on refresh, cleanup on disconnect) and backs them with a well-structured vitest harness. The fixes are correct and the test seam (transport) is a clean design. A few issues below.

Bugs

1. Falsy-zero index in buildRecentFetchUrl and expandTimeRangeUrl — || should be ??

web-components/src/log-list.ts lines 347 and 434:

// line 347 — buildRecentFetchUrl
const timestamp = firstItem?.data?.[this.colIdxMap['timestamp'] || this.colIdxMap['created_at']];
// line 434 — expandTimeRangeUrl
const oldestTimestamp = oldestItem?.data?.[this.colIdxMap['timestamp'] || this.colIdxMap['created_at']];

colIdxMap['timestamp'] === 0 is a valid index (first column) but is falsy, so || falls back to colIdxMap['created_at'], reading the wrong column. buildLoadMoreUrl already uses the correct pattern at line 385:

const timestampCol = this.colIdxMap['timestamp'] !== undefined ? 'timestamp' : 'created_at';

Fix: use ?? or the same !== undefined guard at lines 347 and 434.

2. isNewResetTimer not cleared in disconnectedCallback

web-components/src/log-list.ts around line 696 (the disconnectedCallback timer cleanup block).

updateBatchTimer, liveStreamInterval, scrollEndTimer, and initChartsTimer are all cleared — but isNewResetTimer (introduced in this PR, line 133 / line 644) is not. If the component is unmounted within 4 seconds of a tree change, the timer fires on a detached element and calls this.requestUpdate().

Fix:

if (this.isNewResetTimer) {
  clearTimeout(this.isNewResetTimer);
  this.isNewResetTimer = null;
}

3. expandedAggregates not cleared on refresh-with-empty-result

web-components/src/log-list.ts lines 1087–1099:

When isRefresh=true but tree.length === 0, the code takes the early-return path that clears spanListTree but never reaches the this.expandedAggregates = {} reset at line 1126 (which is inside the non-empty-tree branch). Stale expanded-aggregate rows survive the empty refresh and will re-render under the same keys when the user re-runs a matching query.

Fix: add if (isRefresh) this.expandedAggregates = {}; inside the empty-tree isFullFetch block.

Cleanup / Succinctness

4. private formatCount duplicates formatPatternCount — use the export

web-components/src/log-list.ts lines 615–622 vs web-components/src/log-list-utils.ts line 371:

formatPatternCount was added to utils in this PR and is already imported in log-list.ts (line 30). The private formatCount method is byte-for-byte identical. Six call sites exist (lines 578–585). Replace the private method with the imported util to eliminate the duplicate:

// replace 6x this.formatCount(...) → formatPatternCount(...)
// delete lines 615-622

5. Spinner prematurely hidden when stale load-more is discarded mid-refresh

web-components/src/log-list.ts line 1078 / finally block line 1188:

When a stale load-more's transport resolves and hits the gen !== this.fetchGeneration guard (line 1078), it returns from the try-block — but finally still runs and calls showLoadingSpinner(false), removing the spinner DOM node. If a concurrent full refresh is still in-flight with isLoading=true, the spinner disappears mid-fetch. Low-severity UX glitch, but worth tracking.

6. colIdxMap always overwritten while logsColumns is fetch-guarded

web-components/src/log-list.ts line 1114:

if (isFullFetch) this.logsColumns = meta.cols;   // guarded — good
this.colIdxMap = meta.colIdxMap;                  // unguarded — always overwrites

logsColumns is intentionally preserved on load-more/live-tick so user column reordering survives. colIdxMap is the server-side index map that drives all data lookups and should stay in sync with whatever the server returns, so overwriting it is arguably correct — but the asymmetry means a reordered logsColumns and an updated colIdxMap can temporarily diverge (different column at the same index). Consider a comment explaining the intent, or guard it symmetrically.

Code Review

This PR fixes a solid cluster of real bugs in the log explorer: dedup across paginated/live pages, stale load-more contamination via generation counter, per-kind loading guard reset, live-stream listener cleanup, column persistence across ticks, and the chart sequencer for out-of-order responses. The test harness with a transport seam and controllable IntersectionObserver is the right approach. A few actionable items below:

Bug — isNewResetTimer not cleared in disconnectedCallback

disconnectedCallback diligently clears 5 other timers (updateBatchTimer, liveStreamInterval, scrollEndTimer, initChartsTimer) but misses isNewResetTimer introduced in this PR. If the component is removed within 4 s of new rows arriving, the timer fires on a dead component, mutates spanListTree objects, and calls requestUpdate() on a disconnected element.

Fix — add to disconnectedCallback alongside the other timer cleanups:

if (this.isNewResetTimer) { clearTimeout(this.isNewResetTimer); this.isNewResetTimer = null; }

Simplification — handleColumnsChanged: use pick (already imported)

pick is imported from lodash at line 8. The new for-loop is equivalent to a one-liner using it:

// current
for (const c of Object.keys(this.columnMaxWidthMap)) if (!next.includes(c)) delete this.columnMaxWidthMap[c];

// simpler — same semantics, uses the already-imported util
this.columnMaxWidthMap = pick(this.columnMaxWidthMap, next);

Simplification — dedupeById: use _.uniqBy (lodash already imported in same file)

log-list-utils.ts already imports from lodash (import { get } from 'lodash'). The custom Set-based implementation can be replaced:

// current — comma-expression side-effect inside ternary
export const dedupeById = <T extends { id: string }>(items: T[]): T[] => {
  const seen = new Set<string>();
  return items.filter((it) => (it.id == null || seen.has(it.id) ? false : (seen.add(it.id), true)));
};

// simpler
import { get, uniqBy } from 'lodash';
export const dedupeById = <T extends { id: string }>(items: T[]): T[] =>
  uniqBy(items.filter((it) => it.id != null), 'id');

The null-guard (it.id == null) also silently drops rows with a null/undefined id rather than passing them through — worth an explicit comment at minimum, or a console.warn in dev if this should never happen given the type constraint.

Minor — chartFetchSeq module state persists between test runs

chartFetchSeq is module-level and is never reset. The three tests in chart-fetch-seq.test.ts avoid interference by using distinct chart IDs (chart1, chart2, chartX/Y), but this is fragile — a future test reusing any of those IDs would get an unexpected initial sequence number. Consider exporting a _resetForTest() helper, or accepting the risk given the IDs are intentionally different.

Not bugs (for the record):

• shouldBufferRecent threshold change (> 30 → > 0) is correct: in newest-first mode new rows enter at the top, so any positive scrollTop means the user has scrolled away from the live edge.
• cursorFromTimestamp thresholds (1e17 for ns, 1e14 for µs) are correct for current wall-clock epochs.
• hasMore = false on an empty load-more page is intentional and correct (the PR description explains why).

Code Review

This PR fixes a solid cluster of real bugs in the LogList explorer and dashboard widgets — the fetchGeneration guard, per-kind loading-guard reset, live-stream listener/interval cleanup, dedupe boundary row, and the chart-fetch sequencer are all genuine and well-targeted fixes. The new vitest harness with a transport seam and controllable IntersectionObserver is a good addition. A few issues survived verification:

1. TupleSections not in default-extensions — will not compile

src/Models/Telemetry/Telemetry.hs:1143

WritePgOnly -> singleLeg "pg" This (,0) =<< ...
WriteTfOnly -> singleLeg "tf" That (0,) =<< ...

Telemetry.hs only has {-# LANGUAGE StrictData #-} at the top, and TupleSections is absent from default-extensions in monoscope.cabal. The (,0) / (0,) syntax requires TupleSections — without it the module won't compile. Either add TupleSections to default-extensions or add {-# LANGUAGE TupleSections #-} to Telemetry.hs.

2. buildRecentFetchUrl still uses new Date(timestamp) — same ns/µs bug the PR set out to fix

web-components/src/log-list.ts:351

const date = new Date(timestamp);
date.setTime(date.getTime() + 10); // Add 10ms
url.searchParams.set('from', date.toISOString());

buildLoadMoreUrl was correctly updated to use cursorFromTimestamp(timestamp, -10), but buildRecentFetchUrl still passes the raw timestamp to new Date(). A nanosecond epoch (1700000000000000000) becomes a year-~55000 from date, so live-tail silently returns nothing — the same stall the PR describes fixing. The fix is the same one-liner:

url.searchParams.set('from', cursorFromTimestamp(timestamp, +10));

3. liveStreamInterval not set to null after clearInterval on mode-switch

web-components/src/log-list.ts:630-633

if (changedProperties.has('mode') && this.isAggregate && this.liveStreamInterval) {
  clearInterval(this.liveStreamInterval);
  this.isLiveStreaming = false;
  // missing: this.liveStreamInterval = null;
}

Every other teardown site (lines 161, 168, 687) sets this.liveStreamInterval = null after clearInterval. This one doesn't. After clearing, liveStreamInterval holds a stale (but truthy) timer ID. If the user switches back to logs and re-enables live-tail, handleLiveToggle's guard if (!this.liveStreamInterval) sees a truthy value and skips setInterval — live streaming appears active but never polls.

4. expandTimeRangeUrl sets cursor as raw String(oldestTimestamp) — same ns/µs bug

web-components/src/log-list.ts:437

url.searchParams.set('cursor', String(oldestTimestamp));

Unlike buildLoadMoreUrl (which now calls cursorFromTimestamp), expandTimeRangeUrl passes the raw timestamp as a string. For nanosecond-epoch timestamps, String(1700000000000000000) sends a numeric string the server may interpret as-is, producing a year-55000 cursor and returning zero results when "Show earlier events" is clicked.

url.searchParams.set('cursor', cursorFromTimestamp(oldestTimestamp, 0));

5. expandTimeRange incorrectly set to true on empty live-tail tick

web-components/src/log-list.ts:1087-1088

this.hasMore = isLoadMore ? false : meta.hasMore || false;
this.expandTimeRange = !this.hasMore;

When a live-tail tick (isRecentFetch=true) returns 0 rows, meta.hasMore is typically false (no forward rows), so this.expandTimeRange = true. This causes the "Show earlier events" button to flash on every quiet 5-second tick during live-streaming, even though history hasn't been exhausted. The fix is to guard the assignment:

if (!isRecentFetch) this.expandTimeRange = !this.hasMore;

6. dedupeById O(n_total) on every live-tail tick — consider deduping only new rows

web-components/src/log-list.ts:1309 (inside addWithFlipDirection)

dedupeById always builds a Set from the full merged array. For load-more this is fine (one-shot). For live-tail (every 5 s), after an hour with a high-throughput stream the tree can hold thousands of rows, all re-scanned each tick. The existing rows are already unique from the previous tick's dedup — only new rows need to be checked against existing IDs. Not a blocker, but worth noting as the tree grows.

Minor — dedupeById comma-operator side effect is hard to read

web-components/src/log-list-utils.ts:245

return items.filter((it) => (it.id == null || seen.has(it.id) ? false : (seen.add(it.id), true)));

The (seen.add(it.id), true) comma-operator pattern works correctly but a future maintainer may read the seen.add call as "dead code" and remove it. The explicit form is clearer:

return items.filter((it) => {
  if (it.id == null || seen.has(it.id)) return false;
  seen.add(it.id);
  return true;
});

Also note: == null is the only loose-equality check in the file; a comment explaining "catches both null and undefined" would prevent an eqeqeq lint fix from silently breaking it.

Code Review

This PR fixes a solid cluster of real bugs in the log explorer (duplicate rows, stale load-more, live-tail leaks, column-persist regression, chart sequencer) and adds a good regression-test harness with a transport seam. The Haskell cleanup (intercalate/toStrict unqualified via Relude, TupleSections) is correct and safe.

A few issues found:

Bug — hasMore clobbered on empty recent-fetch

web-components/src/log-list.ts (empty-tree branch, isRecentFetch path)

this.hasMore = isLoadMore ? false : meta.hasMore || false;

When isRecentFetch=true and the live-tail poll returns an empty page, this evaluates to meta.hasMore || false. The recent endpoint doesn't carry backward-pagination context, so it commonly returns hasMore: false (or omits it). That silently sets this.hasMore = false, hiding the load-more affordance for older data for the rest of the session.

The PR already guards expandTimeRange with if (!isRecentFetch) — the same treatment is needed for hasMore:

this.hasMore = isLoadMore ? false : isRecentFetch ? this.hasMore : (meta.hasMore || false);

The existing test 'an empty recent fetch does not turn on expandTimeRange' passes meta: { hasMore: false } but never asserts on this.hasMore, so this path is untested.

Inconsistency — colIdxMap not guarded like logsColumns

web-components/src/log-list.ts (~line 1118)

if (isFullFetch) this.logsColumns = meta.cols;   // ← guarded
this.colIdxMap = meta.colIdxMap;                  // ← always overwrites

colIdxMap is used to index into every row's data[] array (including already-rendered rows from the initial fetch), and is also read by buildRecentFetchUrl/buildLoadMoreUrl for cursor math. For the server to return a different column order on page 2 is admittedly unlikely, but the asymmetry with the logsColumns guard is a latent footgun. Applying the same guard (if (isFullFetch) this.colIdxMap = meta.colIdxMap) makes the two consistent and safe.

Simplification — dedupeById dead null-check + lodash already available

web-components/src/log-list-utils.ts

if (it.id == null || seen.has(it.id)) return false;  // null-check is dead

The generic constraint <T extends { id: string }> guarantees id is always a string, so it.id == null can never be true under the type. The guard silently drops rows rather than surfacing a type violation.

Also, uniqBy from lodash (already a dependency) does exactly this with less code. At call sites in log-list.ts you could use uniqBy(result, 'id') directly; or at minimum remove the dead null check from dedupeById.

Stale loadedCount after buffer flush

web-components/src/log-list.ts (handleRecentConcatenation / handleRecentClick)

this.loadedCount = this.spanListTree.length is set inside fetchData (after merge). When buffered recent rows are flushed into spanListTree by the user clicking the "N new" pill, handleRecentConcatenation appends to spanListTree but never updates loadedCount or calls updateRowCountDisplay(). The row-count badge stays stale until the next fetch.

Minor — transport is an unintentionally public field

web-components/src/log-list.ts (~line 179)

transport: (url: string) => Promise<{ tree: any[]; meta: any }> = (url) => this.workerFetch(url);

No access modifier makes this part of the element's public API. Any script holding a reference to the element can silently replace it. Prefixing with _ or using a protected//* @internal */ annotation would communicate the intent.

Positive notes

• The fetchGeneration guard correctly serialises full fetches via the isLoading mutex at the top of fetchData — the guard doesn't need to cover full fetches because they're already serialized.
• Moving worker + listener setup to connectedCallback / disconnectedCallback is the right fix for the HTMX remount leak.
• chart-fetch-seq.ts is clean and the sequencer tests are a nice pattern.
• Replacing the hand-rolled canvas mock with vitest-canvas-mock is a good call.

Code Review

This PR fixes a solid cluster of real bugs in the log explorer — the fetchGeneration guard, per-kind loading-flag reset, dedup, lifecycle cleanup, and cursorFromTimestamp all address concrete user-visible problems and the regression-test harness is a genuine improvement. A few issues surfaced during review:

Bug — disconnectedCallback drops in-flight promises; isLoading gets stuck permanently

disconnectedCallback calls this.workerCallbacks.clear() after terminating the worker. The 120s timeout correctly no-ops (map entry is gone), but the Promise inside workerFetch never settles — neither resolves nor rejects. fetchData's finally block (which resets isLoading/isFetchingRecent/isLoadingMore) therefore never runs.

connectedCallback resets logsColumns, spanListTree, hasMore, etc., but does not reset the loading flags. On reconnect, fetchInitialData → fetchData hits the if (this.isLoading) return guard at line 1060 and immediately returns. The component loads nothing until the page is reloaded.

// connectedCallback — add before fetchInitialData():
this.isLoading = false;
this.isFetchingRecent = false;
this.isLoadingMore = false;

Bug — live-tail silently stops after a chart zoom (from > to)

buildRecentFetchUrl now correctly keeps to for bounded historical ranges, but handleChartZoom also writes a past to into the URL (e.g. the end of the zoomed window). If live-tail is active, the next 5-second tick sets from = newest loaded row + 10ms (a recent timestamp) while keeping to = the historical end. PostgreSQL BETWEEN with an inverted range returns zero rows and no error, so live-tail silently produces nothing until the user reloads.

A minimal guard in buildRecentFetchUrl:

// After setting 'from', if 'to' is now earlier than 'from', drop it.
const fromParam = url.searchParams.get('from');
const toParam = url.searchParams.get('to');
if (fromParam && toParam && new Date(toParam) < new Date(fromParam))
  url.searchParams.delete('to');

Bug — colIdxMap updated on every fetch, logsColumns only on full fetch

this.colIdxMap = meta.colIdxMap runs unconditionally (line 1118), but this.logsColumns = meta.cols is gated on isFullFetch (line 1117). If the server ever returns a different colIdxMap on a live-tail tick or load-more (server deploy mid-session, column set change), the two fall out of sync: cell values are read from the new index map while headers still reflect the old column list.

Either gate colIdxMap update on isFullFetch too, or keep both updated on every fetch (accepting that user column reorders are reset on ticks). The current asymmetry is the one state that leads to silent mismatches.

Simplification — dedupeById reimplements _.uniqBy (lodash is a runtime dep)

Lodash is in dependencies, already imported in log-list-utils.ts, and used extensively in log-list.ts. dedupeById can be replaced:

// before
export const dedupeById = <T extends { id: string }>(items: T[]): T[] => {
  const seen = new Set<string>();
  return items.filter((it) => {
    if (it.id == null || seen.has(it.id)) return false;
    seen.add(it.id);
    return true;
  });
};

// after (same semantics — null-id guard preserved)
import { uniqBy } from 'lodash';
export const dedupeById = <T extends { id: string }>(items: T[]): T[] =>
  uniqBy(items.filter((it) => it.id != null), 'id');

Minor — shouldBufferRecent 0-px threshold may false-positive on high-DPI / virtualizer jitter

Lowering from scrollTop > 30 to scrollTop > 0 is conceptually correct (buffer on any intentional scroll), but sub-pixel rendering on high-DPI screens and lit-virtualizer re-anchor micro-adjustments can leave scrollTop at 1–2px without any user intent. The practical effect is the "N new" pill appearing for users who are pinned to the top but whose scroller has fractional offset. A threshold of scrollTop > 2 or scrollTop > 5 would still fix the original "arbitrary 30px" problem while tolerating rendering jitter.

Nit — get import from lodash in log-list-utils.ts appears unused

import { get } from 'lodash' is at line 4 of log-list-utils.ts but get is not called anywhere in the file (a comment at line 134 even notes direct array access is preferred). Can be removed.

The Haskell changes (toStrict/intercalate unqualified, TupleSections) are all correct — both resolve via Relude's re-exports and GHC2024's TupleSections. The test harness architecture (transport seam, deferredTransport, controllable IntersectionObserver) is clean and well thought out.

Review: fix(log-explorer) — load-more, live-tail, column & chart bugs

This is a solid cluster of targeted fixes with excellent test coverage. The transport seam, fetchGeneration guard, per-kind loading-guard reset, and listener teardown are all well-reasoned. A few things worth addressing:

Performance — O(n²) dedup in addWithFlipDirection

web-components/src/log-list.ts (around line 1316)

dedupeById is called on the full merged array (existing + new) on every load-more append:

return dedupeById(result); // [...current, ...newData]

dedupeById allocates a fresh Set and scans the entire merged list each call. With p pages of n rows each, total work is O(p² × n). For a user paginating through hundreds of pages, this degrades noticeably.

Fix: maintain a seenIds: Set<string> on the component, reset it on a full fetch, and only filter the incoming page before merging:

// in fetchData, on isFullFetch: this.seenIds = new Set();
// in addWithFlipDirection:
const fresh = newData.filter(r => !this.seenIds.has(r.id));
fresh.forEach(r => this.seenIds.add(r.id));
return isRecentFetch ? [...fresh, ...current] : [...current, ...fresh];

Correctness — buildRecentFetchUrl keeping to silences live-tail on trailing-edge bounded ranges

web-components/src/log-list.ts (around line 356)

The PR deliberately keeps to to avoid live-tail pulling rows past the user's upper bound. The comment is correct for the general case, but there's an edge case: if the user has scrolled to (or near) the newest row in a historical range, then from = newest_timestamp + 10 ms can land at or past to. Every subsequent 5 s tick sends a request where from ≥ to, the server returns zero rows, fetchData sees isRecentFetch + empty tree and early-returns silently. The live banner stays on indefinitely with no data and no error.

Consider clamping from to stay below to when both are set, or detecting the from > to condition and disabling live-streaming automatically with a toast.

Cleanup — dedupeById null-guard is dead code given the type constraint

web-components/src/log-list-utils.ts (line 249)

if (it.id == null || seen.has(it.id)) return false; // == null catches both null and undefined

T extends { id: string } means id is always a string at the call site — the == null branch can never fire. If null/undefined IDs are a real runtime possibility (e.g. from untyped server data), widen the constraint to id: string | null | undefined so the intent is explicit rather than hidden behind the comment. Otherwise, drop the guard.

Cleanup — handleColumnsChanged uses Array.includes in a loop

web-components/src/log-list.ts (around line 1210)

for (const c of Object.keys(this.columnMaxWidthMap)) if (!next.includes(c)) delete this.columnMaxWidthMap[c];

Array.includes is O(n) per iteration. Trivial to fix:

const nextSet = new Set(next);
for (const c of Object.keys(this.columnMaxWidthMap)) if (!nextSet.has(c)) delete this.columnMaxWidthMap[c];

Note — intercalate/toStrict de-qualification in Telemetry.hs is fine

Both names come via import Relude hiding (ask), so no build risk there.

The lifecycle teardown, fetchGeneration guard, column-preservation on load-more/live-tick, and per-kind loading-guard reset are all correct and well-tested. The test harness design (transport seam + controllable IntersectionObserver) is a good pattern to keep building on.

Code Review

This PR fixes a cluster of real, user-visible bugs in the LogList component and dashboard widgets, with a solid accompanying test suite. The fetchGeneration guard, per-kind loading-flag resets, seenIds dedup, worker teardown, and the chart-fetch sequencer are all well-reasoned fixes. The ensureTemplateDatabase refactor is a meaningful improvement — eliminating the connect-to-locked-template anti-pattern by stamping the checksum as a DB comment.

A few things worth addressing:

1. Spurious network request after live-tail auto-stop (log-list.ts ~line 379)

buildRecentFetchUrl calls stopLiveStream(...) but does not early-return. The setInterval callback always calls fetchData(buildRecentFetchUrl(), false, true), so after auto-stopping, one final fetch fires with from ≥ to (a guaranteed-empty range). The empty-recent-fetch handler manages it gracefully (no expandTimeRange flip, etc.), but it's a wasteful extra round-trip on every auto-stop event.

if (!isNaN(toMs) && Date.parse(from) >= toMs) {
  this.stopLiveStream('Reached the end of the selected time range — live tail paused.');
  return url.toString(); // ← add this so the caller doesn't proceed to fetchData
}

The test (buildRecentFetchUrl stops live-tail…) correctly asserts isLiveStreaming === false, but does not assert that no fetch is dispatched.

2. deferredTransport.settle is position-sensitive (log-list-harness.ts)

settle(i, data) resolves pending[i] by array index. The current tests guard against implicit fetches by replacing fetchInitialData with a no-op, which keeps the index stable — but this coupling is invisible to future test authors. If a new internal call is added (or the auto-fetch suppression is removed), indices shift silently and tests pass for the wrong promises.

A label-based or queue-based API would be safer:

// e.g. settle('loadMore', ...) or settle('recent', ...)
// or just pop from the front: const settle = (data: any[]) => pending.shift()!.resolve(...)

3. Unquoted identifier in DDL (TestUtils.hs, lines 357 / 375 / 377 / 398)

templateDbName is concatenated directly into ALTER DATABASE, DROP DATABASE, CREATE DATABASE, and COMMENT ON DATABASE without Postgres identifier quoting:

"ALTER DATABASE " <> templateDbName <> " WITH " <> clause
"CREATE DATABASE " <> templateDbName

It's safe today because templateDbName = "monoscope_test_template" is hardcoded with safe characters. But Postgres will silently fold an unquoted identifier to lowercase if it ever contains uppercase letters, and will syntax-error on hyphens. Quoting defensively:

let qi t = "\"" <> T.replace "\"" "\"\"" t <> "\""

(The migrationChecksum in the COMMENT value is already properly single-quote-escaped — just the identifier interpolation is missing.)

4. transport is public with any return types (log-list.ts, line 551)

The test seam has no access modifier (TypeScript class members default to public) and uses any for both tree elements and meta:

transport: (url: string) => Promise<{ tree: any[]; meta: any }> = (url) => this.workerFetch(url);

This becomes part of the component's public API. A /** @internal */ JSDoc comment plus a tighter return type (Promise<{ tree: EventLine[]; meta: LogMeta }>) would signal intent and avoid accidental external dependencies on it.

5. Buffer path (recentDataToBeAdded) accumulates already-seen IDs (log-list.ts, line 907)

addWithFlipDirection dedupes within the buffer itself via dedupeById, but does not filter against seenIds. So when live-tail returns the inclusive-boundary row (already in the tree), it lands in the buffer and is held there until handleRecentConcatenation calls mergeIntoTree, which then correctly drops it via seenIds. The final result is always correct, but the buffer can hold stale entries indefinitely if the user stays scrolled away for many ticks.

This is a minor efficiency concern, not a correctness bug, since the buffer is typically small and short-lived.

Overall the approach is solid. The test harness (fakeTransport / deferredTransport / stubFetch / controllable IntersectionObserver) is a good addition that makes the concurrency and lifecycle scenarios verifiable without hitting real network or worker constraints.

Code Review

This PR is a solid, well-documented fix of a cluster of interconnected pagination/live-tail/lifecycle bugs. The transport seam, fetchGeneration guard, per-kind loading guards, and seenIds-based O(page) dedup are all the right fixes. Test coverage is thorough and the harness design is clean. A few things to tighten up:

Confirmed bugs

1. buildRecentFetchUrl fires the "live tail paused" toast when the user is NOT live-streaming
log-list.ts:379

buildRecentFetchUrl is now side-effectful — it calls stopLiveStream(message) when from >= to. stopLiveStream fires the toast unconditionally on any non-null message. A user on a bounded historical range who clicks the "Load newer events" button (line 2133 calls buildRecentFetchUrl() directly) while NOT live-streaming will see "Reached the end of the selected time range — live tail paused" even though they never had live tail on.

Fix — guard the stopLiveStream call with this.isLiveStreaming:

if (!isNaN(toMs) && Date.parse(from) >= toMs && this.isLiveStreaming)
  this.stopLiveStream('Reached the end of the selected time range — live tail paused.');

The interval path still stops correctly (it only fires when isLiveStreaming is true); the manual-click path no longer shows a spurious toast.

2. tryAny in runSharedProducer catches too broadly
src/Pkg/Queue.hs:163

tryAny catches SomeException, which includes async exceptions (ThreadKilled, StackOverflow, UserInterrupt). Receiving killThread during a web request returns Left "thread killed" to the handler instead of propagating, breaking clean shutdown semantics. It also silently swallows programming errors (pattern-match failures, error "...") as Kafka warning strings.

Since the library throws a concrete K.KafkaError as an IO exception, try @K.KafkaError is sufficient and does not catch anything else:

runSharedProducer appCtx act =
  try @K.KafkaError (runErrorNoCallStack @K.KafkaError (runSharedKafkaProducer appCtx act)) >>= \case
    Left e  -> pure $ Left (toText (show e))
    Right r -> pure $ first (toText . show) r

If broader coverage is needed (e.g. a broker-down path throws a different exception type), prefer catches with an async-exception re-throw guard over the blanket tryAny.

Minor / cleanup

3. || 0 should be ?? 0 in chart-fetch-seq.ts:12

|| 0 masks a stored 0; ?? 0 expresses that only undefined (key absent) triggers the default — matching the module's invariant that all live values are positive integers:

const seq = (chartFetchSeq[chartId] = (chartFetchSeq[chartId] ?? 0) + 1);

4. Buffer dedup is O(buffer) per 5-second tick
log-list.ts:1345 (addWithFlipDirection)

The comment calls the buffer a "small bounded list" but it is unbounded — a user who scrolls away with live-tail running for 10 minutes at 50 rows/tick accumulates ~6 000 entries, and each tick re-scans the full array via dedupeById(orderMerge(...)). The seenIds set is already available; using a parallel set for the buffer (or reusing seenIds directly, since buffered rows will enter the tree on flush) reduces this to O(page) per tick, matching mergeIntoTree.

5. colIdxMap overwritten on every fetch; logsColumns frozen after first fetch
log-list.ts:1143

if (isFullFetch) this.logsColumns = meta.cols;
this.colIdxMap = meta.colIdxMap;   // runs on every live tick / load-more

If the server returns a colIdxMap with a shifted column index after a schema migration, the index map updates but logsColumns stays frozen, silently showing wrong cell values for every column after the insertion point. Freezing colIdxMap to isFullFetch as well (or asserting the key-set matches) would close this gap. Low practical risk if schema migrations always reload the page, but worth noting.