Skip to content

feat(logs): rewrite logs page with server-side search - #1465

Merged
harshithmohan merged 4 commits into
masterfrom
feat/logs-page
Sep 14, 2026
Merged

harshithmohan merged 4 commits into
masterfrom
feat/logs-page

Conversation

@harshithmohan

@harshithmohan harshithmohan commented Sep 13, 2026

Copy link
Copy Markdown
Member

Summary

Rewrites the Logs page: the live tail (LogLiveView) is now a pure SignalR feed, and a new LogSearchView adds full-history server-side search via Logging/Range/Read with infinite scroll. The React Query log module moves from react-query/logs/ to react-query/logging/ and the log event type is expanded (logger/caller/exception).

Changes

Added

  • LogSearchView.tsx — debounced search text and/or level chips switch the page from the live tail to full-history search, with virtualized infinite scroll (effect-triggered fetchNextPage). Search text supports the server's filter DSL:

    Prefix Meaning
    c:foo contains
    =:foo equals
    ^:foo starts with
    $:foo ends with
    ~:foo fuzzy match (case-insensitive)
    *:foo regex (/pattern/i for ignore-case)

    ! (negate) and # (ignore case) can be appended after the mode in either order (c!#:foo, each once). Bare text is a case-insensitive contains; inputs that don't match the DSL grammar are wrapped as c#:<text> so the server never rejects them.

  • LogLevelChip.tsx, LogRow.tsx — level filter chips and a shared log row (timestamps formatted exactly once, here).

  • useVirtualizerScrollRectWorkaround.ts — shared hook wrapping the TanStack Virtual #634 measurement fix (Firefox/Chrome), replacing duplicated inline code in both views.

Fixed

  • LogLiveView.tsx — scroll-to-bottom lock no longer disables itself while logs load: the old scrollTop snapshot race was tripped by the virtualizer's measurement corrections; it now checks distance-from-bottom instead (scrollbar drags are caught too).
  • queries.ts — query keys derive a stable, content-sensitive string from the level Set (a Set serializes to {} in the key hash, so level changes previously didn't refetch); empty message/level params are omitted from requests rather than sent as empty strings.

Removed

  • react-query/logs/ — replaced by react-query/logging/ with expanded LogEventType (logger/caller/exception); LogLineType dropped from common.ts.

Screenshots

Live tail

logs-live-tail

Level filter (server-side search)

logs-level-filter

Search

logs-search

@harshithmohan
harshithmohan marked this pull request as draft September 13, 2026 12:19
- Derive stable, content-sensitive query key from level Set (Set serializes
  to {} in the hash, so level filters never refetched during search)
- Omit empty message/level params instead of sending empty DSL operands
- Skip DSL wrapping for empty search strings
- Fix DSL prefix regex to match server grammar (mode char first, at most
  one ! and one # modifier in either order)
- Gate full-screen spinner to initial fetch only; drop redundant isPending
- Replace render-phase side effect + useMemo debounce with an effect
  keyed on the trailing virtual index (React Compiler rule compliance)
- Format timestamps once in LogRow; remove formatStamp/formatTimestamps
  from the data layer
- Route level-chip filtering to the server search view; drop unused
  activeLevels/onClearFilters props from LogLiveView
- Extract virtualizer scrollRect workaround into
  useVirtualizerScrollRectWorkaround hook
- Fix hasMore off-by-one before first load (!= null)
- Sync filtersActive with debounced search value
The old detection compared scrollTop snapshots 50ms apart, so the
virtualizer's measurement corrections (rows measured != 36px estimate)
and programmatic scrollToIndex events looked like a user scroll-up and
disabled the lock as soon as logs loaded.

Replace it with a bottom check: while locked, the container is only not
at the bottom if the user scrolled up. Also catches scrollbar drags and
drops the throttle machinery.
@harshithmohan
harshithmohan marked this pull request as ready for review September 14, 2026 10:27
@hidden4003

Copy link
Copy Markdown
Member

PR Review: #1465 — feat(logs): rewrite logs page with server-side search

📋 Summary

PR 1465 splits the Logs page into a pure SignalR live tail (LogLiveView) and a new server-side, full-history search view (LogSearchView) with a small text DSL, virtualized infinite scroll, and shared row/chip components. It also fixes two real pre-existing bugs (a scroll-lock race and a Set-in-query-key hashing bug) along the way. The PR is well self-reviewed (two fixup commits already address earlier review findings), CI is green, and the code is consistent with the project's established conventions. Overall quality is high; the issues below are refinements, not blockers.

🚨 Critical Issues & Security (Blockers)

None detected. No secrets, no unsafe HTML injection (event.Message/event.Exception render as plain JSX text), no new/risky dependencies, search text goes through axios's normal params serialization rather than manual URL concatenation.

🏗️ Architecture & Pattern Checks

  • React Query module layout, hooks usage, and Redux/store imports all match AGENTS.mdreact-query/logging/{queries,types}.ts, no useMemo/useCallback/React.memo (React Compiler compliant), typed imports via import type, @/ alias throughout.
  • getNextPageParam returns null instead of undefined (src/core/react-query/logging/queries.ts:100) — every other infinite query in this codebase (file, filter, series, group, tmdb, duplicate-files, missing-episodes, release-management) explicitly returns undefined to signal "no more pages," and one existing consumer (DuplicateFilesUnrecognizedTab.tsx) drives its scroll logic off React Query's own hasNextPage. This PR instead returns the raw NextOffset (which can be null) and has LogSearchView recompute hasMore itself from the last page's NextOffset != null. Functionally guarded, but a deviation from how every other infinite list in the repo does it. Is this intentional, or should it follow the existing undefined-returning / hasNextPage-driven pattern?
  • Pre-existing (not introduced by this PR): the logging SignalR connection is still a component-local useEffect + HubConnectionBuilder, rather than going through the centralized SignalR-as-Redux-middleware architecture AGENTS.md describes for /signalr/aggregate. Carried over unchanged from the old logs/queries.ts — flagging for awareness only.
  • Button used as a bare semantic wrapper for LogLevelChip toggles (LogsPage.tsx:722-729) — no buttonType/buttonSize passed, so LogLevelChip owns all styling and Button supplies only click/keyboard/tooltip semantics. Every other Button call site in the repo sets buttonType. Reasonable, but a first for the codebase — worth confirming it's intentional.

💡 Suggestions & Refactoring

1. Align getNextPageParam/hasMore with the rest of the codebase:

// Before
getNextPageParam: lastPage => lastPage.NextOffset,
// After
getNextPageParam: lastPage => lastPage.NextOffset ?? undefined,

and prefer the hook's own hasNextPage/isFetchingNextPage over hand-deriving hasMore, matching DuplicateFilesUnrecognizedTab.tsx.

2. Add a regression test for the DSL prefix matcher. AGENTS.md calls out "auto-match logic/regexes" as worth regression protection, and hasDslPrefix/toServerSearch (LogSearchView.tsx:483-485) is the same category of risk — regex-driven parsing mirroring a server grammar, easy to silently break. Extract to an exported module and add a tests/pages/logs/... unit test covering the prefix/modifier combinations.

3. UX consistency (not a bug): live tail is chronological-ascending; search results are descending: true (newest-first). Reasonable per-view, but flips reading direction when switching modes — worth a deliberate call.

🔍 Nitpicks

  • level: levelKey || undefined / message: search || undefined (queries.ts:96-97) technically diverge from the ??-over-|| guideline, but || is required here since these are non-nullable strings and ?? wouldn't catch the empty-string case. A short inline comment would prevent a future "fix" that breaks the omission logic.
  • oxlint-disable-next-line no-bitwise / no-param-reassign suppressions are used correctly and match existing style.
  • Good catches on the Set-in-query-key hashing bug and the scroll-lock race — both real, well-explained fixes.

@hidden4003

Copy link
Copy Markdown
Member

Closes #221

@harshithmohan
harshithmohan merged commit 9a411aa into master Sep 14, 2026
3 checks passed
@harshithmohan
harshithmohan deleted the feat/logs-page branch September 14, 2026 16:50
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.

2 participants