-
Notifications
You must be signed in to change notification settings - Fork 1
β‘ Bolt: [performance improvement] Memoize LogEntryItem in LogsView #154
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| ## 2024-05-14 - Initial Setup | ||
| **Learning:** Initial journal setup. | ||
| **Action:** None. |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,7 +2,7 @@ | |||||
| * Logs view component β logs viewer with filtering. | ||||||
| */ | ||||||
|
|
||||||
| import { useEffect, useMemo, useState } from "react"; | ||||||
| import React, { useEffect, useMemo, useState } from "react"; | ||||||
| import { useApp } from "../AppContext"; | ||||||
| import type { LogEntry } from "../api-client"; | ||||||
| import { formatTime } from "./shared/format"; | ||||||
|
|
@@ -18,6 +18,69 @@ const TAG_COLORS: Record<string, { bg: string; fg: string }> = { | |||||
| websocket: { bg: "rgba(20, 184, 166, 0.15)", fg: "rgb(20, 184, 166)" }, | ||||||
| }; | ||||||
|
|
||||||
| /** | ||||||
| * Extracted, memoized log entry item. | ||||||
| * Prevents re-rendering all log lines when the search input value changes. | ||||||
| */ | ||||||
| const LogEntryItem = React.memo(function LogEntryItem({ | ||||||
| entry, | ||||||
| }: { | ||||||
| entry: LogEntry; | ||||||
| }) { | ||||||
| return ( | ||||||
| <div | ||||||
| className="font-mono text-xs px-2 py-1 border-b border-border flex gap-2 items-baseline" | ||||||
| data-testid="log-entry" | ||||||
| > | ||||||
| {/* Timestamp */} | ||||||
| <span className="text-muted whitespace-nowrap"> | ||||||
| {formatTime(entry.timestamp, { fallback: "β" })} | ||||||
| </span> | ||||||
|
|
||||||
| {/* Level */} | ||||||
| <span | ||||||
| className={`font-semibold w-[44px] uppercase text-[11px] ${ | ||||||
| entry.level === "error" | ||||||
| ? "text-danger" | ||||||
| : entry.level === "warn" | ||||||
| ? "text-warn" | ||||||
| : "text-muted" | ||||||
| }`} | ||||||
| > | ||||||
| {entry.level} | ||||||
| </span> | ||||||
|
|
||||||
| {/* Source */} | ||||||
| <span className="text-muted w-16 overflow-hidden text-ellipsis whitespace-nowrap text-[11px]"> | ||||||
| [{entry.source}] | ||||||
| </span> | ||||||
|
|
||||||
| {/* Tag badges */} | ||||||
| <span className="inline-flex gap-0.5 shrink-0"> | ||||||
| {(entry.tags ?? []).map((t: string) => { | ||||||
| const c = TAG_COLORS[t]; | ||||||
| return ( | ||||||
| <span | ||||||
| key={t} | ||||||
| className="inline-block text-[10px] px-1.5 py-px rounded-lg mr-0.5" | ||||||
| style={{ | ||||||
| background: c ? c.bg : "var(--bg-muted)", | ||||||
| color: c ? c.fg : "var(--muted)", | ||||||
| fontFamily: "var(--font-body, sans-serif)", | ||||||
| }} | ||||||
| > | ||||||
| {t} | ||||||
| </span> | ||||||
| ); | ||||||
| })} | ||||||
| </span> | ||||||
|
|
||||||
| {/* Message */} | ||||||
| <span className="flex-1 break-all">{entry.message}</span> | ||||||
| </div> | ||||||
| ); | ||||||
| }); | ||||||
|
|
||||||
| export function LogsView() { | ||||||
| const [searchQuery, setSearchQuery] = useState(""); | ||||||
|
|
||||||
|
|
@@ -163,57 +226,10 @@ export function LogsView() { | |||||
| </div> | ||||||
| ) : ( | ||||||
| filteredLogs.map((entry: LogEntry) => ( | ||||||
| <div | ||||||
| <LogEntryItem | ||||||
| key={`${entry.timestamp}-${entry.source}-${entry.level}-${entry.message}`} | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using the full Note that this is a trade-off: if two log messages are identical for the first 256 characters but differ later, React might not be able to distinguish them correctly if they have the same timestamp, source, and level. However, for logging UIs, this is often an acceptable risk for the performance benefit.
Suggested change
|
||||||
| className="font-mono text-xs px-2 py-1 border-b border-border flex gap-2 items-baseline" | ||||||
| data-testid="log-entry" | ||||||
| > | ||||||
| {/* Timestamp */} | ||||||
| <span className="text-muted whitespace-nowrap"> | ||||||
| {formatTime(entry.timestamp, { fallback: "β" })} | ||||||
| </span> | ||||||
|
|
||||||
| {/* Level */} | ||||||
| <span | ||||||
| className={`font-semibold w-[44px] uppercase text-[11px] ${ | ||||||
| entry.level === "error" | ||||||
| ? "text-danger" | ||||||
| : entry.level === "warn" | ||||||
| ? "text-warn" | ||||||
| : "text-muted" | ||||||
| }`} | ||||||
| > | ||||||
| {entry.level} | ||||||
| </span> | ||||||
|
|
||||||
| {/* Source */} | ||||||
| <span className="text-muted w-16 overflow-hidden text-ellipsis whitespace-nowrap text-[11px]"> | ||||||
| [{entry.source}] | ||||||
| </span> | ||||||
|
|
||||||
| {/* Tag badges */} | ||||||
| <span className="inline-flex gap-0.5 shrink-0"> | ||||||
| {(entry.tags ?? []).map((t: string) => { | ||||||
| const c = TAG_COLORS[t]; | ||||||
| return ( | ||||||
| <span | ||||||
| key={t} | ||||||
| className="inline-block text-[10px] px-1.5 py-px rounded-lg mr-0.5" | ||||||
| style={{ | ||||||
| background: c ? c.bg : "var(--bg-muted)", | ||||||
| color: c ? c.fg : "var(--muted)", | ||||||
| fontFamily: "var(--font-body, sans-serif)", | ||||||
| }} | ||||||
| > | ||||||
| {t} | ||||||
| </span> | ||||||
| ); | ||||||
| })} | ||||||
| </span> | ||||||
|
|
||||||
| {/* Message */} | ||||||
| <span className="flex-1 break-all">{entry.message}</span> | ||||||
| </div> | ||||||
| entry={entry} | ||||||
| /> | ||||||
| )) | ||||||
| )} | ||||||
| </div> | ||||||
|
Comment on lines
226
to
235
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rendering all log entries at once without virtualization can lead to significant performance issues when displaying a large number of logs. Consider integrating a virtualization library such as Recommended solution: import { FixedSizeList as List } from 'react-window';
// ...
<List
height={desiredHeight}
itemCount={filteredLogs.length}
itemSize={rowHeight}
>
{({ index, style }) => (
<div style={style}>
<LogEntryItem entry={filteredLogs[index]} />
</div>
)}
</List>Replace the direct |
||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For better readability and to follow common React patterns, it's best to define props using a separate
typeorinterfaceinstead of an inline object type. This makes the component's signature easier to read and reuse.For example: