feat(admin): live log viewer and searcher in log files page - #845
Conversation
…igureWait to LogFiles.razor
sven-n
left a comment
There was a problem hiding this comment.
Review
Scope: single file, src/Web/AdminPanel/Pages/LogFiles.razor (+322/−37). Adds a side-by-side log terminal with live polling, text filter, level color coding, and a download action; also rewrites the file-list load into a refreshable method and fixes FormatFileSize.
Nice work overall — the FormatFileSize rewrite is a genuine bug fix (the old < 1024 << 10 switch mislabeled small files as KiB and had a dead bytes arm), and reading only the trailing 100 KB is the right instinct for a 4 MiB rolling log. The level tokens ([Error], [Warning], [Information], [Debug]) do match the Serilog outputTemplate in src/Startup/appsettings.json, so the coloring will actually fire.
Correctness / behavior
- Forced scroll breaks live reading.
RefreshLogLinessets_shouldScrollToBottom = trueunconditionally, and the timer calls it every 2 s. A user who scrolls up to read history gets yanked to the bottom every two seconds. Only auto-scroll when the terminal was already at (or near) the bottom, or only on initial select. - Disposal race.
Dispose()disposes the timer, but a callback already in flight can still callInvokeAsync(...)on a disposed component →ObjectDisposedExceptionon the renderer. Add a_disposedguard (or aCancellationTokenSource) checked inside the callback, and preferPeriodicTimerin a background loop overSystem.Threading.Timer. SetupTimeruses??=, so toggling live update twice is safe, but the period/dueTimeis never re-armed. Minor today; fragile if the interval becomes configurable.- Blocking file I/O on the render/timer path.
ReadLastLinesis fully synchronous and runs fromSelectFile(UI circuit) and from the timer. On a slow or remote disk this stalls the circuit. The convention elsewhere in the repo is async; use async reads and make the handlersasync Task. ConfigureAwait(false)in a component is actually the wrong convention inside Blazor components — it drops the renderer's synchronization context. It happens to be harmless here (nothing follows the awaits), but the PR description claims it as adherence to project conventions; it isn't.- Multi-line exceptions lose their color and their filter context.
{Exception}stack traces are separate lines with no[Level]token, so they render in the default gray, and a text filter shows a matching stack frame with no header line. Consider grouping a header line with its continuation lines. - Broad
catches.ReadLastLinescatchesExceptionand injects"Error reading log file: {ex.Message}"into the log data — better to surface that in a distinct UI error field.ScrollToBottomAsync's barecatch { }should be narrowed toJSDisconnectedException(plusTaskCanceledException) so real interop errors aren't silently swallowed.
Performance
GetFilteredLines()is called twice per render (once for the list, once for the "Showing X of Y" counter), each allocating a newList<string>. With live mode that's two full scans plus two allocations every 2 s, plus one per keystroke in the filter box. Compute once into a field on refresh / search change.- The timer calls
StateHasChanged()unconditionally, re-diffing 300<div>s even when the file hasn't changed. Cheap guard: skip ifFileInfo.Length/LastWriteTimeUtcis unchanged.
Project conventions
- Localization regression. The page previously used
Resources.FileName/LastUpdate/Sizethroughout; the new UI hardcodes English:"Log Files","Actions","Log Viewer:","Live","Refresh","Close","Filter log entries...","No log entries found.","No log entries match your filter.","Showing ... lines", and everytitle=. These belong inProperties/Resources.resx(and the translated variants). This is probably the main blocker. JSRuntime.InvokeVoidAsync("eval", ...)is a smell and CSP-hostile. Define a small JS function (or use anElementReferenceplus a module in a.razor.js) instead of shippingeval.- BOM removed from line 1 (
-@page→@page). Every other.razorin the repo is UTF-8-with-BOM; this makes the change show up as a whole-file rewrite in some tools. Please restore it. - Heavy inline styles (
style="color: #ff6b6b...", hardcoded hex backgrounds,!important) rather than CSS classes. Moving these to the site stylesheet keeps theming consistent and the markup readable. - Magic numbers
102400,300,2000are duplicated between code and the user-visible string "Last 300 lines loaded" — hoist them toprivate const.
Security
- No path traversal: the file is chosen from a server-enumerated
FileInfo, never from user-supplied text. Good. - Log content is rendered as text via
@line, so Razor escapes it — no XSS from log payloads. Good. - The page inherits the Admin Panel's existing auth, but note it now streams log contents rather than just linking files; worth confirming the panel is authenticated in the targeted deployments. Not something this PR introduces.
- The description mentions "matching highlight styling" for search hits, but the diff only implements filtering. Either implement highlighting (escaping carefully before injecting
<mark>) or drop the claim.
Tests
No tests. There is no existing Blazor test project, so this matches the status quo, but FormatFileSize and ReadLastLines are static and nearly pure — extracting them into a small helper class would allow covering the boundary cases (0 bytes, exactly 1024, file shorter than 100 KB, offset landing mid-line).
Suggested must-fix before merge
- Localize all new strings via
Resources. - Replace the
evalinterop with a named JS function. - Guard disposal in the timer callback; stop force-scrolling on every live refresh.
- Cache
GetFilteredLines()instead of calling it twice per render. - Restore the UTF-8 BOM.
Generated by Claude Code
…CSP JS module, timer disposal, and scroll UX
sven-n
left a comment
There was a problem hiding this comment.
Re-review of f10a9d9 (after the update)
Thanks for the quick turnaround — most of the previous points are addressed: strings are in Resources, eval is gone in favour of a collocated JS module, IAsyncDisposable + _disposed guards the timer callback, the filtered list is cached in _filteredLines instead of being recomputed twice per render, and the magic numbers are now const. The live-refresh path also skips StateHasChanged() when length/LastWriteTimeUtc are unchanged, which is exactly the guard I hoped for.
However, the new revision introduces three build-breaking defects, all in the resource plumbing.
🔴 Blocking
1. Resources.resx is no longer well-formed XML. The DownloadFile entry is missing its closing </data>:
<data name="DownloadFile" xml:space="preserve">
<value>Download File</value>
<data name="ChatCommands" xml:space="preserve">Verified against the PR head — xml.etree fails with mismatched tag: line 686. This alone fails the build.
2. Duplicate resource keys. The resx already contains Actions (line 129) and Refresh (line 201) on master; the diff adds second <data> entries for both. Duplicate names are a resgen error. Drop the two new entries and just use the existing keys.
3. Resources.Designer.cs is hand-edited and out of sync. It's an <auto-generated> file — the added properties should come from re-running the custom tool, not from manual edits. As it stands the manual additions cover LogViewer…DownloadFile but the generator would also normalize ordering, so the next regeneration will produce a large spurious diff. (Note Actions and Refresh are correctly not re-added here — which is precisely why the resx duplicates in point 2 are wrong.)
4. The JS module path is wrong for this project. MUnique.OpenMU.Web.AdminPanel.csproj is Sdk="Microsoft.NET.Sdk.Web" — an app, not a Razor Class Library. The _content/{PackageId}/… prefix only applies to RCLs, so
await this.JSRuntime.InvokeAsync<IJSObjectReference>("import", "./_content/MUnique.OpenMU.Web.AdminPanel/Pages/LogFiles.razor.js");will 404 at runtime. The existing convention in this repo is ReconnectModal.razor using @Assets["Components/Layout/ReconnectModal.razor.js"], i.e. app-root-relative. Use ./Pages/LogFiles.razor.js (ideally via @Assets[...] for fingerprinting). Because the import is wrapped in a bare catch, this failure is completely silent — _jsModule stays null and scrolling just never happens, with no console clue. That's a good argument for at least logging the exception rather than swallowing it.
🟠 Functional regression from the fix
5. Auto-scroll during live mode is now gone entirely. RefreshLogLines no longer sets _shouldScrollToBottom, and only SelectFile does — so in live mode new lines append but the terminal never follows them, which defeats the purpose of a live tail. The exported isScrolledToBottom in LogFiles.razor.js is never called from C#, which suggests the intended "only auto-scroll if the user is already at the bottom" logic was written but not wired up. Suggested shape:
// in the timer callback, after RefreshLogLines()
this._shouldScrollToBottom = this._jsModule is null
|| await this._jsModule.InvokeAsync<bool>("isScrolledToBottom", "log-terminal");That restores tailing without hijacking a user who has scrolled up.
🟡 Minor
- Missing trailing newline at the end of
LogFiles.razor(\ No newline at end of file). - BOM removed from
Resources.Designer.csline 1 (same issue as the.razorfile last round — that one is fixed now, thanks). Every other file in the repo keeps the UTF-8 BOM. ReadLastLinesis still fully synchronous and runs on the UI circuit / timer thread; theConfigureAwait(false)calls are gone now (good — that was the right call for component code), but async file reads would still be an improvement.ReadLastLinesstill swallows all exceptions into a fake log line ("Error reading log file: …"); a distinct error field would read better and isn't confusable with real log content. That string is also not localized, unlike the rest of the UI now.- Colors and the terminal chrome remain inline styles rather than CSS classes.
- Multi-line exception continuations still render in the default gray and drop out of text filters — not blocking, just a known limitation worth a comment.
Verdict
The design is in good shape now; points 1–4 are mechanical but each one breaks the build or the feature, so they need a fix before this can go in. Point 5 is the one behavioural gap worth another look.
Generated by Claude Code
- Resources.resx: close the unterminated DownloadFile data element, which
made the file invalid XML, and remove the duplicated Actions and Refresh
entries which already exist.
- Resources.Designer.cs: restore the UTF-8 BOM and put the new properties
into the alphabetical order the strongly typed resource builder produces,
so the file matches its generated form again.
- LogFiles.razor: import the collocated script from ./Pages/LogFiles.razor.js.
The _content/{PackageId} prefix only applies to razor class libraries, so
the import failed for this web application and the module was never loaded.
- LogFiles.razor: follow the new entries in live mode again by using the
isScrolledToBottom helper, so the terminal scrolls along unless the user
scrolled up to read the history.
- LogFiles.razor: only catch the expected javascript interop exceptions and
log a failing module import instead of swallowing it silently.
- LogFiles.razor: restore the BOM and the trailing newline.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Vjs6n29WzQx8pGg3KJPGXk
(cherry picked from commit 7be969e)
Live Log Viewer & Searcher in Admin Panel
Implements an in-browser live log viewer and search tool directly within the Admin Panel's Log Files page.
Key Features
Code Quality & Guidelines
this.qualification standards andConfigureAwait(false)async conventions.