Skip to content

feat: adds log explorer component#2283

Merged
eblairmckee merged 8 commits intomasterfrom
pr2/log-explorer-molecule
Mar 13, 2026
Merged

feat: adds log explorer component#2283
eblairmckee merged 8 commits intomasterfrom
pr2/log-explorer-molecule

Conversation

@eblairmckee
Copy link
Contributor

@eblairmckee eblairmckee commented Mar 11, 2026

Summary

  • New LogExplorer component (components/ui/connect/log-explorer.tsx) — pipeline log viewer built with TanStack Table, featuring column filtering (level, component, message text), sorting, pagination, and a detail sheet for individual log entries.
  • react-query/api/logs.tsx — log search hook using two strategies:
    • History mode: React Query (useTanstackQuery) with incremental streaming via a ref + flush interval, so the table populates progressively during the search and query cache handles refetch/staleness.
    • Live mode: standalone streaming hook using ConnectRPC listMessages directly, prepending new messages (newest-first tail mode).
    • Supports server-side pushdown filters on serverful clusters (filterInterpreterCode) with client-side keyJson fallback on serverless.
  • Extracted shared utilities from backend-api.ts:
    • message-converters.tsconvertListMessageData (message deserialization/conversion), previously inline in createMessageSearch. Now shared by both the legacy createMessageSearch and the new useLogSearch.
    • message-table-helpers.tsisFilterMatch (extracted from pipelines-details.tsx) and trimSlidingWindow (extracted from Tab.Messages/index.tsx). Pure functions, zero side-effect imports.
  • Feature-flagged: enableNewPipelineLogs controls whether the pipeline view tab renders LogExplorer (new) or LogsTab (legacy). Defaults to false.
  • Legacy LogsTab improvements: accepts a variant prop for border styling, uses registry Button for the refresh action, and imports shared isFilterMatch instead of defining it locally.

Test plan

  • message-converters.test.ts — unit tests for message deserialization (compression types, encodings, troubleshooting, payload edge cases)
  • message-table-helpers.test.ts — unit tests for isFilterMatch and trimSlidingWindow
  • log-explorer.test.tsx — integration tests for the component (loading, empty state, table rendering, pagination, log levels, error display, refresh, detail sheet)
  • logs.test.tsx — integration test verifying live mode prepends messages (newest first)

Live view

Screenshot 2026-03-11 at 5 46 11 PM

Historical view with filters

Screenshot 2026-03-11 at 5 52 43 PM

@secpanda
Copy link

secpanda commented Mar 11, 2026

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

Comment on lines +367 to +368
<Section borderColor={variant === 'ghost' ? 'transparent' : undefined} minWidth="800px" overflowY="auto">
<div className="mb-6 flex items-center justify-between gap-2">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding variant so we can embed legacy version when feature flag for new log explorer is off

Comment on lines -400 to -412
function isFilterMatch(str: string, m: TopicMessage) {
const lowerStr = str.toLowerCase();
if (m.offset.toString().toLowerCase().includes(lowerStr)) {
return true;
}
if (m.keyJson?.toLowerCase().includes(lowerStr)) {
return true;
}
if (m.valueJson?.toLowerCase().includes(lowerStr)) {
return true;
}
return false;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extracted to shared util

Comment on lines -322 to -362
function trimSlidingWindow({
messages,
maxResults,
pageSize,
currentGlobalPage,
windowStartPage,
virtualStartIndex,
}: {
messages: TopicMessage[];
maxResults: number;
pageSize: number;
currentGlobalPage: number;
windowStartPage: number;
virtualStartIndex: number;
}): { messages: TopicMessage[]; windowStartPage: number; virtualStartIndex: number; trimCount: number } {
const maxWindowSize = maxResults + pageSize;

if (maxResults < pageSize || messages.length <= maxWindowSize) {
return { messages, windowStartPage, virtualStartIndex, trimCount: 0 };
}

const excess = messages.length - maxWindowSize;
const currentLocalPage = Math.max(0, currentGlobalPage - windowStartPage);

// Never trim the page the user is currently viewing or the one before it
const maxPagesToTrim = Math.max(0, currentLocalPage - 1);
const pagesToTrim = Math.min(Math.floor(excess / pageSize), maxPagesToTrim);
const trimCount = pagesToTrim * pageSize;

if (trimCount === 0) {
return { messages, windowStartPage, virtualStartIndex, trimCount: 0 };
}

return {
messages: messages.slice(trimCount),
windowStartPage: windowStartPage + pagesToTrim,
virtualStartIndex: virtualStartIndex + trimCount,
trimCount,
};
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extracted to shared util

@malinskibeniamin
Copy link
Contributor

@claude review

@claude
Copy link

claude bot commented Mar 11, 2026

Claude finished @malinskibeniamin's task in 2m 2s —— View job


Code Review

  • Read CLAUDE.md for repo guidelines
  • Read changed files
  • Analyze code quality, bugs, patterns
  • Post review feedback

Overall this is a well-structured extraction and new molecule. The 59 unit/integration tests provide solid coverage. A few issues found below, ranging from an accessibility bug to minor code quality items.


Bugs

1. Label-for/id mismatch — clicking "Live" label won't toggle the switch

log-explorer.tsx:343–358: The Label has htmlFor="live-view-toggle" but the id="live-view-toggle" is placed on the InfoIcon instead of the Switch. Clicking the label text will not toggle the switch.

// Current (broken)
<Switch checked={liveViewEnabled} onCheckedChange={...} />  {/* no id */}
<Label htmlFor="live-view-toggle">Live</Label>
<InfoIcon id="live-view-toggle" />  {/* wrong element gets the id */}

// Fix: move id to Switch
<Switch id="live-view-toggle" checked={liveViewEnabled} onCheckedChange={...} />
<Label htmlFor="live-view-toggle">Live</Label>

Fix this →

2. m.valueJson assigned twice in convertListMessageData

message-converters.ts:169,178: m.valueJson = valuePayload; appears at both line 169 and line 178. The second assignment is redundant — it looks like a copy/paste artifact from the extraction. Harmless but noisy. Fix this →


Design Concerns

3. loadLargeMessage is a MobX-dependent function inside a "pure" utils file

message-table-helpers.ts:83–109: This function calls createMessageSearch() from backend-api.ts, which instantiates a MobX MessageSearch store. Placing it in a pure utils file alongside isFilterMatch and trimSlidingWindow is inconsistent — callers might reasonably expect everything in this module to be side-effect-free. Consider moving loadLargeMessage to a separate message-search-utils.ts or co-locating it with the MobX search infrastructure.

4. Live mode has unbounded memory growth per session

use-log-search.ts:112–113: In live mode, every matching message is appended to messages state without any cap or sliding window:

setMessages((prev) => [...prev, msg]);

With LIVE_MAX_RESULTS = 1000, up to 1000 messages can accumulate in React state per session (and the hook re-streams on any param change). The PR description mentions Julin's sliding window work, but there's no TODO or guard here. Worth adding a comment noting this is intentional/expected to be addressed.

5. Echoing existing reviewer feedback (magic numbers and BigInt timestamps)

These are already raised by @malinskibeniamin in inline comments — agreeing with both:

  • use-log-search.ts:96: 30 * 60 * 1000 / 30 * 1000 should be named constants (LIVE_TIMEOUT_MS, HISTORY_TIMEOUT_MS)
  • use-log-search.ts:91: BigInt(startTime.getTime()) — if the proto field is a bigint scalar, this works, but it's worth checking whether @bufbuild/protobuf/wkt's timestampFromMs / protoInt64 helpers apply here for consistency with the rest of the proto layer

Minor Code Quality

6. new TextDecoder() instantiated per-message in a hot path

message-converters.ts:114,148: Two new TextDecoder() instances are created inside convertListMessageData, which is called per-message in a streaming loop. TextDecoder is inexpensive to construct but hoisting it to module scope (like JSONBigInt on line 22) is cleaner:

const textDecoder = new TextDecoder();

7. Magic number 150 for TanStack default column size

log-explorer.tsx:395,437: The pattern header.getSize() !== 150 ? header.getSize() : undefined relies on TanStack Table's internal default column size of 150. This is fragile — if a column is intentionally sized at 150, it won't apply. Consider using a named constant or checking columnDef.size !== undefined instead.

8. Fragile CSS class selector in test

log-explorer.test.tsx:65: document.querySelector('.animate-spin') is a fragile way to detect the loading state. Prefer a data-testid on the spinner or check for an accessible role/label. The .animate-spin class is a Tailwind utility that could change independently.


Reviewer Action Items Verification

backend-api.ts extraction is behavior-preserving. The extracted convertListMessageData is called with res.controlMessage.value (the same DataMessage object as before). Default compression is CompressionType.Unknown in both the original inline code and the extracted function's default case. Behavior-equivalent.

Tab.Messages trimSlidingWindow import. The import at Tab.Messages/index.tsx:362 replaces a local copy with the shared util. The function signature and semantics are identical.

LogsTab variant prop default. The variant prop defaults to 'card' and only affects the Section component's borderColor — no visual change on existing pages.

@eblairmckee
Copy link
Contributor Author

@claude

@eblairmckee eblairmckee requested a review from c-julin March 12, 2026 00:57
@claude
Copy link

claude bot commented Mar 12, 2026

Claude finished @eblairmckee's task in 1m 14s —— View job


Researching proto3 timestamp utilities

  • Analyze trigger context
  • Check logs.tsx current BigInt timestamp usage
  • Search codebase for proto3 timestamp utility patterns
  • Provide answer to Blair's question

Answer: BigInt() is correct here — timestampFromMs doesn't apply

Short answer: timestampFromMs is used elsewhere in the codebase, but it's for a different proto field type. BigInt(startTime.getTime()) is the right approach for startTimestamp.

Why: startTimestamp in ListMessagesRequest is defined as int64 in the proto schema (not google.protobuf.Timestamp):

// list_messages_pb.ts:41-43
/**
 * Start offset by unix timestamp in ms (only considered if start offset is set to -4).
 *
 * @generated from field: int64 start_timestamp = 3;
 */
startTimestamp: bigint;

timestampFromMs from @bufbuild/protobuf/wkt creates a Timestamp message object ({ seconds: bigint, nanos: number }) — it's only applicable to google.protobuf.Timestamp message fields. Since startTimestamp is a plain int64 (unix ms), you need BigInt(), not timestampFromMs.

It is an existing pattern: backend-api.ts:2804 uses the identical approach for the same field:

req.startTimestamp = BigInt(searchRequest.startTimestamp);

Where timestampFromMs is used: In fields that are google.protobuf.Timestamp messages — metric-chart.tsx:43-44 and transcript-list-page.tsx:416–493. Different field type entirely.

So @malinskibeniamin's suggestion about proto3 utilities is a reasonable instinct, but doesn't apply here. BigInt(startTime.getTime()) is correct and consistent with the existing codebase pattern for this specific field.

@eblairmckee
Copy link
Contributor Author

@claude can you look at the last 2 commits (merge master and fix for mobx removal) and identify any issues or risks? note there was a recent effort to remove mobx from pipeline-details.tsx page see #2250 and I'm wondering if the useEffect that fetches pipeline logs was migrated correctly

@claude
Copy link

claude bot commented Mar 12, 2026

Claude finished @eblairmckee's task in 6m 25s —— View job


Analysis: last 2 commits (merge master + MobX fix)

  • Read CLAUDE.md
  • Review last 2 commits (merge master + MobX fix)
  • Trace LogsTab useEffect through the merge conflict
  • Check pipeline/index.tsx (new path)
  • Post findings

Summary

The fix in 7f8fe8b was necessary and correct — the merge introduced a genuine runtime bug that the fix resolved. The useEffect migration is sound. Details below.


What the merge broke (137cb89)

The merge conflict in pipelines-details.tsx was resolved in a way that left a ReferenceError:

// After merge (137cb89) — BROKEN
export const LogsTab = ({ pipeline, variant = 'card' }: ...) => {
  // ...
  useEffect(() => {
    // ...
    executeMessageSearch(search, topicName, p.pipeline.id)  // ❌ `p` is not defined!
    //                                       ^^^^^^^^^
  }, [refreshCount]);  // ❌ also missing pipeline.id from deps

The merge kept the new destructured prop signature { pipeline, variant } from this PR, but the conflict resolver pulled p.pipeline.id from the master version. Since p no longer exists as a variable, this would throw ReferenceError: p is not defined whenever LogsTab mounted — effectively breaking log fetching entirely on the legacy route.

What 7f8fe8b fixed

// Fix (7f8fe8b) — CORRECT
+ // biome-ignore lint/correctness/useExhaustiveDependencies: intentional...
  useEffect(() => {
    // ...
-   executeMessageSearch(search, topicName, p.pipeline.id)
+   executeMessageSearch(search, topicName, pipeline.id)
- }, [refreshCount]);
+ }, [refreshCount, pipeline.id]);

Two distinct issues fixed:

  1. Runtime bug: p.pipeline.idpipeline.id (fixes the ReferenceError)
  2. Lint + correctness: adds pipeline.id to the deps array so the effect re-runs if the user navigates between pipelines without unmounting LogsTab

The second fix is actually an improvement over what was in master before the merge — the master version (5143948) also had pipeline.id in the effect body but not in the deps array.


Is the useEffect logic correct now?

Yes. The current implementation is correct:

useEffect(() => {
  searchRef.current?.stopSearch();          // cancel previous
  const search = createMessageSearch();
  searchRef.current = search;
  queueMicrotask(() => setLogState({ messages: [], isComplete: false }));  // reset
  executeMessageSearch(search, topicName, pipeline.id).finally(() => {
    setLogState({ messages: [...search.messages], isComplete: true });
  });
  return () => { search.stopSearch(); };    // cleanup on unmount/dep change
}, [refreshCount, pipeline.id]);            // ✅ correct deps
  • Effect re-runs on manual refresh (refreshCount) or pipeline change (pipeline.id) — correct
  • Cleanup correctly stops in-flight search before starting a new one
  • The search captured in the finally() closure is always the right instance

One pre-existing race condition (not introduced by these commits)

If stopSearch() resolves the old search's promise asynchronously, the old finally(() => setLogState(...)) could fire after the state has been reset, briefly showing stale data from the old pipeline. This existed in the original version too (with setMessages/setIsComplete) and is harmless in practice since the 200ms poll interval will overwrite it quickly.


New path (pipeline/index.tsx) — unaffected

The new pipeline page (pipeline/index.tsx:551-565) doesn't use LogsTab at all when enableNewPipelineLogs is on — it renders <LogExplorer> directly with no useEffect for log fetching; that's handled by useLogSearch. The MobX concern doesn't apply here.

Legacy class component (RpConnectPipelinesDetails) — pre-existing concern

The class component on lines 60–93 still uses pipelinesApi.pipelines (MobX observable) and appGlobal.onRefresh. This is unchanged by this PR. If MobX reactivity has been fully removed, this class component would stop reacting to pipeline list updates — but that's a pre-existing issue from PR #2250, not introduced here.


Bottom line: The MobX removal merge introduced a ReferenceError in LogsTab and 7f8fe8b fixed it correctly. No further issues with the useEffect migration in these two commits.

@eblairmckee
Copy link
Contributor Author

#2250 changes for reference:
Screenshot 2026-03-12 at 9 20 18 AM

@eblairmckee eblairmckee merged commit dc7f118 into master Mar 13, 2026
15 checks passed
@eblairmckee eblairmckee deleted the pr2/log-explorer-molecule branch March 13, 2026 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants