feat(dashboard): stream live audit and usage logs#334
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end realtime dashboard live logs: config flags, an in-process Broker with bounded replay and snapshots, audit/usage live-event emission, SSE handlers (/admin/live/logs, /admin/audit/detail), App wiring, a browser live-logs module with merge logic, UI styles, tests, and docs. ChangesDashboard Live Logs Streaming Feature
Sequence DiagramsequenceDiagram
participant Browser
participant LiveLogsHandler
participant Broker
participant DashboardModule
Browser->>LiveLogsHandler: GET /admin/live/logs?cursor=...
LiveLogsHandler->>Broker: Subscribe(cursor)
Broker-->>LiveLogsHandler: Replay events (filtered)
LiveLogsHandler->>Browser: SSE replay frames
loop live stream
Broker->>LiveLogsHandler: publish Event
LiveLogsHandler->>Browser: SSE Event
Browser->>DashboardModule: parse & applyLiveLogEvent
DashboardModule->>DashboardModule: merge into audit/usage lists
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Greptile SummaryThis PR adds live audit and usage log streaming to the admin dashboard. It changes:
Confidence Score: 4/5This is close, but the reconnect cursor bug should be fixed before merging.
internal/live/broker.go Important Files Changed
Reviews (4): Last reviewed commit: "fix(live): snapshot nested preview data" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/auditlog/logger.go (1)
205-215:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmit a terminal live event when batch persistence fails.
At Line 205,
WriteBatchfailure drops the whole batch, but only the success path emitsLiveEventAuditFlushed. That leaves previously publishedaudit.completedentries stuck as pending in live clients forever.Suggested fix
if err := l.store.WriteBatch(ctx, batch); err != nil { slog.Error("failed to write audit log batch", "error", err, "count", len(batch), ) + for _, entry := range batch { + l.PublishLiveEvent(LiveEventAuditRemoved, entry) + } return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/auditlog/logger.go` around lines 205 - 215, The WriteBatch error path currently logs the failure but does not notify live clients, leaving prior audit.completed events pending; after the slog.Error call in the WriteBatch error branch (where l.store.WriteBatch is invoked), iterate the batch and call l.PublishLiveEvent for each entry with a terminal failure event (e.g., LiveEventAuditFailed) so live clients receive a terminal state; use the same entry objects from batch and the existing l.PublishLiveEvent method to mark them failed (mirroring the success loop that uses LiveEventAuditFlushed) so pending entries are cleared.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Line 121: The documentation lists only defaults for DASHBOARD_LIVE_LOGS_* but
lacks actionable tuning guidance; update the CLAUDE.md entry for
DASHBOARD_LIVE_LOGS_ENABLED / DASHBOARD_LIVE_LOGS_BUFFER_SIZE /
DASHBOARD_LIVE_LOGS_REPLAY_LIMIT / DASHBOARD_LIVE_LOGS_HEARTBEAT_SECONDS to
include one-line guidance for each: when to increase (e.g., high traffic, large
spikes, long client reconnect windows), when to decrease (e.g., memory/latency
constraints), and minimal example thresholds (e.g., buffer size increase for
>1000 msgs/sec, replay limit for long reconnects, heartbeat lower for frequent
client liveness) so operators know how to tune rather than only seeing defaults.
In `@config/config_test.go`:
- Around line 200-217: The test assertions for cfg.Admin.LiveLogs* require a
clean environment; extend the existing clearAllConfigEnvVars helper to also
unset the four new env vars (DASHBOARD_LIVE_LOGS_ENABLED,
DASHBOARD_LIVE_LOGS_BUFFER_SIZE, DASHBOARD_LIVE_LOGS_REPLAY_LIMIT,
DASHBOARD_LIVE_LOGS_HEARTBEAT_SECONDS) so the Admin defaults used by the config
tests (cfg.Admin.EndpointsEnabled, UIEnabled, LiveLogsEnabled,
LiveLogsBufferSize, LiveLogsReplayLimit, LiveLogsHeartbeatSeconds) are
deterministic; update clearAllConfigEnvVars to call os.Unsetenv for those four
keys (or remove them before the test) so the assertions in config_test.go always
run against default values.
In `@internal/admin/handler_audit.go`:
- Around line 176-206: Add Swagger/OpenAPI annotations for the AuditLogDetail
handler to match the pattern used by AuditLog and AuditConversation: annotate
the AuditLogDetail function (AuditLogDetail) with operation summary,
description, tags (e.g., "admin"), parameters (query param "log_id" required),
success response schema (returning an audit log entry / 200), and error
responses (400/404/500). Place these comments immediately above the
AuditLogDetail function declaration so the API generator picks them up and
ensure the parameter name and response type match the types used in
auditlog.LogEntry and the existing audit endpoints.
In `@README.md`:
- Line 277: Update the README config table to include the three missing
dashboard live-log controls: DASHBOARD_LIVE_LOGS_BUFFER_SIZE,
DASHBOARD_LIVE_LOGS_REPLAY_LIMIT, and DASHBOARD_LIVE_LOGS_HEARTBEAT_SECONDS; for
each add the default value, a one-line description (what it controls and a short
hint when to change it), and keep the existing DASHBOARD_LIVE_LOGS_ENABLED row
style so operators can tune buffering, replay bounds and heartbeat behavior from
the main docs entry point.
---
Outside diff comments:
In `@internal/auditlog/logger.go`:
- Around line 205-215: The WriteBatch error path currently logs the failure but
does not notify live clients, leaving prior audit.completed events pending;
after the slog.Error call in the WriteBatch error branch (where
l.store.WriteBatch is invoked), iterate the batch and call l.PublishLiveEvent
for each entry with a terminal failure event (e.g., LiveEventAuditFailed) so
live clients receive a terminal state; use the same entry objects from batch and
the existing l.PublishLiveEvent method to mark them failed (mirroring the
success loop that uses LiveEventAuditFlushed) so pending entries are cleared.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7a1a2521-0b87-4ac6-9c72-194aa67d680e
📒 Files selected for processing (29)
CLAUDE.mdREADME.mdconfig/admin.goconfig/config.goconfig/config_test.gointernal/admin/dashboard/static/css/dashboard.cssinternal/admin/dashboard/static/js/dashboard.jsinternal/admin/dashboard/static/js/modules/audit-list.jsinternal/admin/dashboard/static/js/modules/live-logs.jsinternal/admin/dashboard/static/js/modules/live-logs.test.cjsinternal/admin/dashboard/static/js/modules/workflows.jsinternal/admin/dashboard/static/js/modules/workflows.test.cjsinternal/admin/dashboard/templates/layout.htmlinternal/admin/handler.gointernal/admin/handler_audit.gointernal/admin/handler_live.gointernal/admin/handler_test.gointernal/admin/routes.gointernal/admin/routes_test.gointernal/app/app.gointernal/app/app_test.gointernal/auditlog/auditlog.gointernal/auditlog/constants.gointernal/auditlog/logger.gointernal/auditlog/middleware.gointernal/live/broker.gointernal/live/broker_test.gointernal/usage/logger.gointernal/usage/usage.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/admin/handler_live.go`:
- Around line 17-87: Add unit tests for LiveLogs exercising cursor parsing, type
filtering, replay/reset behavior, heartbeat pings, and live event forwarding by
creating tests that call Handler.LiveLogs with controlled inputs; specifically,
add *_test.go cases that (1) pass an invalid cursor string to ensure liveCursor
error handling, (2) set query "types" to verify liveTypeFilter excludes/accepts
events, (3) supply a mocked/standalone liveBroker Subscribe result where
sub.Reset is true and sub.Replay contains events to assert writeLiveEvent sends
a reset and replayed events, and (4) provide a Subscribe with an Events channel
and Heartbeat interval to assert periodic heartbeat events and forwarding of
live events. Implement a test liveBroker stub (implementing Enabled, Subscribe,
LatestSeq, Heartbeat) and use an echo request/response recorder to capture SSE
output for assertions, ensuring to close the Events channel to end the handler
loop.
- Around line 101-125: The current liveTypeFilter(raw string) +
liveLogTypeFilter.matches(eventType string) treats an input like "types=foo" (no
valid tokens) the same as no filter and returns true for all events; fix by
distinguishing "no filter provided" from "filter provided but no valid tokens":
change liveLogTypeFilter to carry a boolean (e.g., provided) or equivalent
sentinel set by liveTypeFilter when raw is non-empty, set provided=true only if
raw was given (even if no tokens were valid), and then update matches to return
true when provided==false (no filter specified), but return false when
provided==true and the internal map is empty (filter provided but contained no
valid tokens); update liveTypeFilter(raw string) to trim raw and set the
provided flag accordingly and populate the map only with valid tokens.
In `@internal/live/broker.go`:
- Around line 245-248: When the replay buffer exceeds b.bufferSize, avoid
allocating a new slice; instead shift entries left in-place with copy(b.events,
b.events[drop:]), zero out the now-unused tail slots to avoid retaining
references (e.g. for i := b.bufferSize; i < len(b.events); i++ { b.events[i] =
Event{} }), and then reslice with b.events = b.events[:b.bufferSize]; replace
the current append([]Event(nil), ...) reallocation with this in-place trim using
the identifiers b.events, b.bufferSize, and drop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 98702627-7b01-4603-81ae-effb459b69e8
📒 Files selected for processing (5)
internal/admin/handler_live.gointernal/app/app.gointernal/app/app_test.gointernal/live/broker.gointernal/live/broker_test.go
Comment on lines
+17
to
+87
| // LiveLogs handles GET /admin/live/logs. | ||
| func (h *Handler) LiveLogs(c *echo.Context) error { | ||
| if h.liveBroker == nil || !h.liveBroker.Enabled() { | ||
| return handleError(c, featureUnavailableError("live logs are unavailable")) | ||
| } | ||
|
|
||
| cursor, err := liveCursor(c.QueryParam("cursor")) | ||
| if err != nil { | ||
| return handleError(c, err) | ||
| } | ||
| filter := liveTypeFilter(c.QueryParam("types")) | ||
| sub := h.liveBroker.Subscribe(cursor) | ||
| if sub == nil { | ||
| return handleError(c, featureUnavailableError("live logs are unavailable")) | ||
| } | ||
| defer sub.Close() | ||
|
|
||
| res := c.Response() | ||
| // SSE responses are intentionally long-lived; keep disconnect detection via writes. | ||
| _ = http.NewResponseController(res).SetWriteDeadline(time.Time{}) | ||
| res.Header().Set(echo.HeaderContentType, "text/event-stream") | ||
| res.Header().Set(echo.HeaderCacheControl, "no-cache, no-transform") | ||
| res.Header().Set(echo.HeaderConnection, "keep-alive") | ||
| res.Header().Set("X-Accel-Buffering", "no") | ||
| res.WriteHeader(http.StatusOK) | ||
|
|
||
| if sub.Reset { | ||
| if err := writeLiveEvent(res, live.Event{ | ||
| Seq: h.liveBroker.LatestSeq(), | ||
| Type: live.EventReset, | ||
| }); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| for _, event := range sub.Replay { | ||
| if !filter.matches(event.Type) { | ||
| continue | ||
| } | ||
| if err := writeLiveEvent(res, event); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| ticker := time.NewTicker(h.liveBroker.Heartbeat()) | ||
| defer ticker.Stop() | ||
|
|
||
| ctx := c.Request().Context() | ||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return nil | ||
| case event, ok := <-sub.Events: | ||
| if !ok { | ||
| return nil | ||
| } | ||
| if !filter.matches(event.Type) { | ||
| continue | ||
| } | ||
| if err := writeLiveEvent(res, event); err != nil { | ||
| return err | ||
| } | ||
| case <-ticker.C: | ||
| if err := writeLiveEvent(res, live.Event{ | ||
| Seq: h.liveBroker.LatestSeq(), | ||
| Type: live.EventHeartbeat, | ||
| }); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Add targeted tests for cursor/type parsing and SSE replay/reset flow.
This endpoint introduces multiple control-path branches (invalid cursor, type filtering, reset replay, heartbeat/live forwarding), but file-level coverage is currently missing in this PR context.
As per coding guidelines: **/*_test.go: Add or update tests for behavior changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/admin/handler_live.go` around lines 17 - 87, Add unit tests for
LiveLogs exercising cursor parsing, type filtering, replay/reset behavior,
heartbeat pings, and live event forwarding by creating tests that call
Handler.LiveLogs with controlled inputs; specifically, add *_test.go cases that
(1) pass an invalid cursor string to ensure liveCursor error handling, (2) set
query "types" to verify liveTypeFilter excludes/accepts events, (3) supply a
mocked/standalone liveBroker Subscribe result where sub.Reset is true and
sub.Replay contains events to assert writeLiveEvent sends a reset and replayed
events, and (4) provide a Subscribe with an Events channel and Heartbeat
interval to assert periodic heartbeat events and forwarding of live events.
Implement a test liveBroker stub (implementing Enabled, Subscribe, LatestSeq,
Heartbeat) and use an echo request/response recorder to capture SSE output for
assertions, ensuring to close the Events channel to end the handler loop.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/admin/dashboard/static/js/modules/audit-list.js`:
- Around line 285-290: The function formatDurationNs can produce "NaN s" for
non-numeric inputs; modify formatDurationNs to coerce ns to a Number (e.g.,
const v = Number(ns)) and guard with Number.isFinite(v) (or isFinite(v)) before
doing threshold comparisons, returning '-' (or another sentinel) for non-finite
values; then use v in the existing <=0 / <1000000 / <1000000000 branches so only
real numeric durations are formatted.
In `@internal/admin/dashboard/static/js/modules/live-logs.js`:
- Around line 233-236: The code always decrements auditLog.total by 1 when
replacing auditLog.entries with next, which miscounts when multiple entries were
removed; compute the actual removedCount as (previousEntriesLength -
next.length) before assignment (use the current this.auditLog.entries length or
store it in a temp like prevLen), then set this.auditLog.total = Math.max(0,
Number(this.auditLog.total || 0) - removedCount) and finally assign
this.auditLog.entries = next so total reflects how many entries were actually
removed.
- Around line 79-83: The loop in live-logs parsing only checks for '\n\n' so
CRLF-separated frames (\r\n\r\n) are missed; update the delimiter detection in
the while loop inside the code that slices the buffer (the block that calls
this.handleLiveLogsFrame(frame)) to detect either '\n\n' or '\r\n\r\n' (for
example by using a regex /\r?\n\r?\n/ to find the first match and use
match.index and match[0].length to compute splitAt and the slice offset) so
frames are correctly extracted regardless of LF or CRLF line endings.
In `@internal/admin/handler_audit.go`:
- Around line 190-219: Add unit/handler tests covering AuditLogDetail: test the
missing log_id query path returning 400 (call Handler.AuditLogDetail and assert
handleError/invalid request), test auditReader == nil path returning
503/feature-unavailable, test GetLogByID returning (nil, nil) yields 404, test
GetLogByID returning an error yields the propagated error, and test the
successful 200 path where auditReader.GetLogByID returns an entry and
auditLogResponse enriches usage (assert response JSON equals the single entry).
Use mocks/stubs for h.auditReader.GetLogByID and h.auditLogResponse (or inject a
test auditReader and auditLogResponse helper) to simulate each flow and verify
the correct status codes and responses for AuditLogDetail.
In `@internal/admin/handler_live.go`:
- Around line 60-61: Guard against non-positive heartbeat values returned by
h.liveBroker.Heartbeat() before calling time.NewTicker: read the value into a
variable (e.g., hb := h.liveBroker.Heartbeat()), check if hb <= 0 and replace it
with a safe default (e.g., time.Second) or return early, then call
time.NewTicker(hb). Ensure ticker variable and defer ticker.Stop() are only used
when a ticker is created.
In `@internal/auditlog/logger.go`:
- Around line 73-77: Write currently calls l.PublishLiveEvent synchronously
before the non-blocking send to l.buffer, which can make Write block if a
publisher is slow; change this by decoupling live-event emission: introduce a
bounded buffered queue (e.g., liveEventQueue on the logger struct) and have
Write push events into that queue using a non-blocking select (default/drop) so
Write stays non-blocking, and run a dedicated background goroutine that reads
from liveEventQueue and calls l.PublishLiveEvent; apply the same non-blocking
enqueue pattern to all other synchronous PublishLiveEvent sites (the other
blocks around lines 101-112 and 210-218) and add optional metric/log when an
event is dropped.
In `@internal/usage/logger.go`:
- Around line 73-77: The inline calls to l.publishLiveEvent (e.g., in Write
around LiveEventUsageCompleted/LiveEventUsageFailed and the other occurrences at
100-111 and 209-216) can block request handling; instead make publishing
fire-and-forget by introducing a single buffered, bounded channel (e.g.,
l.livePublishChan) and a dedicated background worker goroutine that consumes
that channel and calls publishLiveEvent; in the Write path (and the other
locations) replace direct calls to l.publishLiveEvent with a non-blocking send
to l.livePublishChan (use select { case l.livePublishChan <- event: default: /*
drop or metric increment to indicate backpressure */ }) so publishing has
bounded backpressure and never blocks the request write path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e243389d-ca84-44ea-a25d-0ceb155a857b
📒 Files selected for processing (21)
CLAUDE.mdREADME.mdconfig/config_test.gointernal/admin/dashboard/static/css/dashboard.cssinternal/admin/dashboard/static/js/dashboard.jsinternal/admin/dashboard/static/js/modules/audit-list.jsinternal/admin/dashboard/static/js/modules/audit-list.test.cjsinternal/admin/dashboard/static/js/modules/live-logs.jsinternal/admin/dashboard/static/js/modules/live-logs.test.cjsinternal/admin/dashboard/templates/page-audit-logs.htmlinternal/admin/handler_audit.gointernal/admin/handler_live.gointernal/admin/handler_live_test.gointernal/auditlog/auditlog.gointernal/auditlog/auditlog_test.gointernal/auditlog/logger.gointernal/live/broker.gointernal/live/broker_test.gointernal/usage/logger.gointernal/usage/usage.gointernal/usage/usage_test.go
Comment on lines
+190
to
+219
| func (h *Handler) AuditLogDetail(c *echo.Context) error { | ||
| logID := strings.TrimSpace(c.QueryParam("log_id")) | ||
| if logID == "" { | ||
| return handleError(c, core.NewInvalidRequestError("log_id is required", nil)) | ||
| } | ||
| if h.auditReader == nil { | ||
| return handleError(c, featureUnavailableError("audit log detail is unavailable")) | ||
| } | ||
|
|
||
| entry, err := h.auditReader.GetLogByID(c.Request().Context(), logID) | ||
| if err != nil { | ||
| return handleError(c, err) | ||
| } | ||
| if entry == nil { | ||
| return handleError(c, core.NewNotFoundError("audit log not found: "+logID)) | ||
| } | ||
|
|
||
| response, err := h.auditLogResponse(c.Request().Context(), &auditlog.LogListResult{ | ||
| Entries: []auditlog.LogEntry{*entry}, | ||
| Total: 1, | ||
| Limit: 1, | ||
| }) | ||
| if err != nil { | ||
| return handleError(c, err) | ||
| } | ||
| if len(response.Entries) == 0 { | ||
| return handleError(c, core.NewNotFoundError("audit log not found: "+logID)) | ||
| } | ||
| return c.JSON(http.StatusOK, response.Entries[0]) | ||
| } |
Contributor
There was a problem hiding this comment.
Add handler tests for the new AuditLogDetail behavior before merge.
This introduces a new public endpoint path (400/404/200 flows plus usage enrichment reuse), but there are no corresponding test additions in the provided changes, and this file is reported at 0% patch coverage.
As per coding guidelines **/*_test.go: “Add or update tests for behavior changes”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/admin/handler_audit.go` around lines 190 - 219, Add unit/handler
tests covering AuditLogDetail: test the missing log_id query path returning 400
(call Handler.AuditLogDetail and assert handleError/invalid request), test
auditReader == nil path returning 503/feature-unavailable, test GetLogByID
returning (nil, nil) yields 404, test GetLogByID returning an error yields the
propagated error, and test the successful 200 path where auditReader.GetLogByID
returns an entry and auditLogResponse enriches usage (assert response JSON
equals the single entry). Use mocks/stubs for h.auditReader.GetLogByID and
h.auditLogResponse (or inject a test auditReader and auditLogResponse helper) to
simulate each flow and verify the correct status codes and responses for
AuditLogDetail.
Comment on lines
+60
to
+61
| ticker := time.NewTicker(h.liveBroker.Heartbeat()) | ||
| defer ticker.Stop() |
Contributor
There was a problem hiding this comment.
Guard ticker creation against non-positive heartbeat values.
time.NewTicker panics on <= 0 durations, and the handler currently uses broker heartbeat directly.
💡 Suggested fix
- ticker := time.NewTicker(h.liveBroker.Heartbeat())
+ heartbeat := h.liveBroker.Heartbeat()
+ if heartbeat <= 0 {
+ heartbeat = 15 * time.Second
+ }
+ ticker := time.NewTicker(heartbeat)
defer ticker.Stop()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ticker := time.NewTicker(h.liveBroker.Heartbeat()) | |
| defer ticker.Stop() | |
| heartbeat := h.liveBroker.Heartbeat() | |
| if heartbeat <= 0 { | |
| heartbeat = 15 * time.Second | |
| } | |
| ticker := time.NewTicker(heartbeat) | |
| defer ticker.Stop() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/admin/handler_live.go` around lines 60 - 61, Guard against
non-positive heartbeat values returned by h.liveBroker.Heartbeat() before
calling time.NewTicker: read the value into a variable (e.g., hb :=
h.liveBroker.Heartbeat()), check if hb <= 0 and replace it with a safe default
(e.g., time.Second) or return early, then call time.NewTicker(hb). Ensure ticker
variable and defer ticker.Stop() are only used when a ticker is created.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/admin/dashboard/static/js/modules/audit-list.js`:
- Around line 203-206: The live-prepend gate (auditLogAllowsLiveEntries)
currently only checks offset and field filters; update it to also prevent live
prepending when a custom date range is active or when the custom range does not
include now. In the auditLogAllowsLiveEntries(payload) function add a check for
this.customStartDate || this.customEndDate (or validate that the provided
customStartDate/customEndDate encompass the current time) and return false if a
custom range is set that excludes now, so live audit rows are only prepended
when no custom date range is in effect (or when the range explicitly includes
the current timestamp).
In `@internal/admin/dashboard/static/js/modules/live-logs.js`:
- Around line 392-397: The code calls this.mergeLiveAuditEntry(payload,
'audit.detail') which replaces the audit row object but then clears
_detail_loading on the old `entry` reference (leaving the live row stuck);
change the flow so the merged/replacement object is the one whose
`_detail_loading` gets cleared — e.g. have mergeLiveAuditEntry return the merged
entry (or lookup the replacement it created) and then set
mergedEntry._detail_loading = false (instead of setting entry._detail_loading),
or update mergeLiveAuditEntry itself to ensure it preserves/clears
`_detail_loading` on the final stored object.
In `@internal/live/broker_test.go`:
- Around line 234-237: The test currently ranges over sub.Events with an
unbounded loop (received := 0; for range sub.Events { received++ }) which can
hang if the channel is never closed; change the loop to be bounded or
timeout-protected by either: a) iterating until an expected count (e.g., for
received < expected { select { case <-sub.Events: received++ case
<-time.After(timeout): t.Fatalf("timed out waiting for events") } }) or b) using
a context with deadline and select on ctx.Done() vs sub.Events; update the loop
around sub.Events accordingly to ensure the test always exits.
In `@internal/live/broker.go`:
- Around line 287-324: The updateActiveSnapshotsLocked logic uses a single
derived key (auditActiveKey/usageActiveKey) so events keyed by id vs request_id
end up in separate slots; change updateActiveSnapshotsLocked to normalize and
operate on both alias keys: compute both primaryKey and aliasKey (derived from
id and request_id), when reading previous entries check both maps
(activeAudit/activeUsage) and merge into the chosen canonical entry via
mergeEventData, when setting store the event under the canonical key and delete
any alternate alias entry, and when deleting (EventAuditFailed/EventUsageFailed
etc.) remove both alias keys; apply the same normalization fix to the other
similar block later in the file that handles the same active snapshot lifecycle.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 935bc44c-48f0-44b3-bd5c-7b276ea0a2cb
📒 Files selected for processing (11)
internal/admin/dashboard/static/js/modules/audit-list.jsinternal/admin/dashboard/static/js/modules/audit-list.test.cjsinternal/admin/dashboard/static/js/modules/live-logs.jsinternal/admin/dashboard/static/js/modules/live-logs.test.cjsinternal/admin/handler_test.gointernal/auditlog/auditlog_test.gointernal/auditlog/logger.gointernal/live/broker.gointernal/live/broker_test.gointernal/usage/logger.gointernal/usage/usage_test.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/admin/dashboard/static/js/modules/audit-list.js (1)
203-206:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDisable live-row prepend when a custom date range is active.
Line 203 currently allows live preview merging even for custom historical ranges, which can show out-of-range rows and skew filtered results.
Proposed fix
auditLogAllowsLiveEntries(payload) { return payload && Number(payload.offset || 0) === 0 && - !this.auditSearch && !this.auditMethod && !this.auditStatusCode && !this.auditStream; + !this.auditSearch && !this.auditMethod && !this.auditStatusCode && !this.auditStream && + !this.customStartDate && !this.customEndDate; },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/admin/dashboard/static/js/modules/audit-list.js` around lines 203 - 206, The current auditLogAllowsLiveEntries function lets live rows prepend even when a custom date range is set; update the function to also require that no custom date range is active by returning false if any date-range filters are present (e.g., this.auditDateFrom or this.auditDateTo); specifically, in auditLogAllowsLiveEntries(payload) keep the existing checks (payload && Number(payload.offset||0)===0 && !this.auditSearch && !this.auditMethod && !this.auditStatusCode && !this.auditStream) and add an extra clause that both date-from and date-to range fields are falsy (for example: && !this.auditDateFrom && !this.auditDateTo) so live-row prepend is disabled when a custom historical range is active.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@internal/admin/dashboard/static/js/modules/audit-list.js`:
- Around line 203-206: The current auditLogAllowsLiveEntries function lets live
rows prepend even when a custom date range is set; update the function to also
require that no custom date range is active by returning false if any date-range
filters are present (e.g., this.auditDateFrom or this.auditDateTo);
specifically, in auditLogAllowsLiveEntries(payload) keep the existing checks
(payload && Number(payload.offset||0)===0 && !this.auditSearch &&
!this.auditMethod && !this.auditStatusCode && !this.auditStream) and add an
extra clause that both date-from and date-to range fields are falsy (for
example: && !this.auditDateFrom && !this.auditDateTo) so live-row prepend is
disabled when a custom historical range is active.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9494333e-c5cd-49d5-a2eb-685287d1a406
📒 Files selected for processing (11)
internal/admin/dashboard/static/js/modules/audit-list.jsinternal/admin/dashboard/static/js/modules/audit-list.test.cjsinternal/admin/dashboard/static/js/modules/live-logs.jsinternal/admin/dashboard/static/js/modules/live-logs.test.cjsinternal/admin/handler_test.gointernal/auditlog/auditlog_test.gointernal/auditlog/logger.gointernal/live/broker.gointernal/live/broker_test.gointernal/usage/logger.gointernal/usage/usage_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/admin/dashboard/static/js/modules/live-logs.js`:
- Around line 176-186: The merge path handling 'audit.detail' is incorrectly
driving the lifecycle: when eventType === 'audit.detail' do not compute or
overwrite lifecycle fields—avoid calling
liveAuditStateAfter/liveAuditEventFlushed for this event and preserve
previous._live_state, previous._audit_flushed and previous._live_pending; only
set patch._detail_loaded = true. Update the merge code (the block that computes
liveState, auditFlushed and builds patch in mergeLiveAuditEntry/wherever used)
to short-circuit for 'audit.detail' so it does not flip _live_pending or mark
_audit_flushed, and apply the same change to the other occurrences mentioned
(the blocks at the other ranges).
- Around line 187-190: When merging incoming live patches into currentEntries we
overwrite previously loaded detail payloads because the code replaces
previous.data when patch.data is falsy; update the merge logic in the block that
builds merged (where currentEntries.splice is called) so that if patch.data is
missing but previous.data exists we keep previous.data (i.e., prefer patch.data
when present, otherwise retain previous.data), and ensure the merged object is
created from previous and patch in that order (const merged = { ...previous,
...patch }) with an explicit conditional to preserve previous.data when
patch.data is undefined to avoid dropping audit.detail-loaded request/response
bodies.
- Around line 156-159: The auditLiveInsertAllowed() gate currently ignores the
custom date window and should reject live inserts when a customStartDate or
customEndDate is active; update auditLiveInsertAllowed() to also require that
this.customStartDate and this.customEndDate are not set (e.g., include &&
!this.customStartDate && !this.customEndDate) so live SSE rows are not prepended
while a historical audit range is selected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6cf927b7-9127-47b5-b476-f49c94d44cd7
📒 Files selected for processing (6)
internal/admin/dashboard/static/js/modules/audit-list.jsinternal/admin/dashboard/static/js/modules/audit-list.test.cjsinternal/admin/dashboard/static/js/modules/live-logs.jsinternal/admin/dashboard/static/js/modules/live-logs.test.cjsinternal/live/broker.gointernal/live/broker_test.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/admin/dashboard/static/js/modules/live-logs.js (1)
199-204:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMerge
datafields when both exist to preserve loaded detail payloads.When
previous.datacontains loaded detail (request_headers, response_body, etc.) andpatch.dataarrives with partial data, the spread{ ...previous, ...patch }replacesprevious.dataentirely. The condition on line 201 only covers whenpatch.datais undefined, but not when both exist.Proposed fix
if (index >= 0) { const merged = { ...previous, ...patch }; - if (patch.data === undefined && previous.data !== undefined) merged.data = previous.data; + if (previous.data && patch.data) { + merged.data = { ...previous.data, ...patch.data }; + } else if (patch.data === undefined && previous.data !== undefined) { + merged.data = previous.data; + } currentEntries.splice(index, 1, merged); this.auditLog.entries = [...currentEntries]; return merged; }The same fix should be applied to line 181 in the
audit.detailpath.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/admin/dashboard/static/js/modules/live-logs.js` around lines 199 - 204, The merge logic inside the live-logs update should combine nested data objects instead of replacing them: when merging previous and patch (the variables named previous, patch, merged and the array currentEntries), detect if both previous.data and patch.data are plain objects and set merged.data to a shallow merge of those two (e.g., {...previous.data, ...patch.data}) so existing loaded detail fields (request_headers, response_body, etc.) are preserved when partial patches arrive; keep the existing fallback that uses previous.data if patch.data is undefined and update this.auditLog.entries accordingly. Apply the same change to the analogous audit.detail merge path referenced in the comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@internal/admin/dashboard/static/js/modules/live-logs.js`:
- Around line 199-204: The merge logic inside the live-logs update should
combine nested data objects instead of replacing them: when merging previous and
patch (the variables named previous, patch, merged and the array
currentEntries), detect if both previous.data and patch.data are plain objects
and set merged.data to a shallow merge of those two (e.g., {...previous.data,
...patch.data}) so existing loaded detail fields (request_headers,
response_body, etc.) are preserved when partial patches arrive; keep the
existing fallback that uses previous.data if patch.data is undefined and update
this.auditLog.entries accordingly. Apply the same change to the analogous
audit.detail merge path referenced in the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 778305fa-79c7-4494-a9e5-d0abf7d98e3a
📒 Files selected for processing (2)
internal/admin/dashboard/static/js/modules/live-logs.jsinternal/admin/dashboard/static/js/modules/live-logs.test.cjs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/admin/dashboard/static/js/modules/live-logs.js (1)
156-160:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReuse the same “range includes now” gate for SSE inserts.
auditLogWithLiveEntries()now allows live rows when a custom range still includes the current time, but this path still disables SSE inserts for any custom range. A filter likeyesterday → tomorrowwill therefore show the initial live rows fromfetchAuditLog()and then stop receiving subsequent updates.Proposed fix
auditLiveInsertAllowed() { - return this.auditLog && this.auditLog.offset === 0 && - !this.auditSearch && !this.auditMethod && !this.auditStatusCode && !this.auditStream && - !this.customStartDate && !this.customEndDate; + const dateRangeAllowsNow = typeof this.auditLiveDateRangeAllowsNow === 'function' + ? this.auditLiveDateRangeAllowsNow() + : (!this.customStartDate && !this.customEndDate); + return this.auditLog && this.auditLog.offset === 0 && + !this.auditSearch && !this.auditMethod && !this.auditStatusCode && !this.auditStream && + dateRangeAllowsNow; },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/admin/dashboard/static/js/modules/live-logs.js` around lines 156 - 160, auditLiveInsertAllowed() currently disallows SSE inserts whenever a customStartDate/customEndDate are set, causing live updates to stop even if the custom range includes now; change auditLiveInsertAllowed to reuse the same "range includes now" gate used by auditLogWithLiveEntries() (or call that helper/condition) so it returns true when the effective range includes the current time even if customStartDate/customEndDate exist, keeping the original checks for auditSearch/auditMethod/auditStatusCode/auditStream and auditLog.offset === 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/auditlog/logger.go`:
- Around line 118-135: The code currently calls snapshotLiveLogEntry
unconditionally which does a deep clone even when no live publisher exists;
change enqueueLiveEvent to first check whether a publisher is attached (inspect
l.liveEvents and the publisher callback publishLiveEventNow) and return
immediately if neither exists, and only call snapshotLiveLogEntry when you will
actually publish or enqueue (i.e., when l.liveEvents != nil or
publishLiveEventNow != nil); update enqueueLiveEvent to perform this
early-return check and move the snapshotLiveLogEntry call below that guard so
cloning is avoided when no publisher is attached.
---
Duplicate comments:
In `@internal/admin/dashboard/static/js/modules/live-logs.js`:
- Around line 156-160: auditLiveInsertAllowed() currently disallows SSE inserts
whenever a customStartDate/customEndDate are set, causing live updates to stop
even if the custom range includes now; change auditLiveInsertAllowed to reuse
the same "range includes now" gate used by auditLogWithLiveEntries() (or call
that helper/condition) so it returns true when the effective range includes the
current time even if customStartDate/customEndDate exist, keeping the original
checks for auditSearch/auditMethod/auditStatusCode/auditStream and
auditLog.offset === 0.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 44cb7426-88fc-4895-b482-a41a22c375df
📒 Files selected for processing (10)
internal/admin/dashboard/static/js/modules/audit-list.jsinternal/admin/dashboard/static/js/modules/audit-list.test.cjsinternal/admin/dashboard/static/js/modules/dashboard-layout.test.cjsinternal/admin/dashboard/static/js/modules/live-logs.jsinternal/admin/dashboard/static/js/modules/live-logs.test.cjsinternal/admin/dashboard/templates/audit-pane.htmlinternal/auditlog/auditlog_test.gointernal/auditlog/logger.gointernal/live/broker.gointernal/live/broker_test.go
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/auditlog/auditlog_test.go`:
- Around line 443-454: The test double-closes the channel publisher.release when
the timeout branch triggers; remove the redundant close or guard it with a
sync.Once so publisher.release is closed exactly once. Specifically, update the
timeout branch that currently does close(publisher.release) and _ =
logger.Close() to either omit close(publisher.release) (leave the single close
just after the select) or introduce a sync.Once (e.g., var releaseOnce
sync.Once) and replace both close(publisher.release) calls with
releaseOnce.Do(func(){ close(publisher.release) }), keeping the subsequent
logger.Close() handling as-is.
In `@internal/auditlog/logger.go`:
- Around line 204-217: The current cloneLiveJSONValue function does an expensive
json.Marshal/json.Unmarshal round-trip for every value; replace or augment it to
perform a structural deep-copy for common runtime types (at least map[string]any
and []any) to avoid serialization overhead: detect if value is a map[string]any
or []any and recursively copy keys/elements (preserving nested maps/slices and
primitive values) inside cloneLiveJSONValue, falling back to the JSON round-trip
only for unknown types; keep the function signature and behavior but implement
the fast-path deep-copy to reduce allocations and CPU for large audit bodies.
In `@internal/live/broker.go`:
- Around line 607-616: copyRawData currently performs a shallow copy which
preserves references to nested maps/slices and can lead to published events
changing if entry.RawData is mutated later; update copyRawData (used by
usagePreviewFromEntry/entry.RawData) to perform a deep copy of nested structures
(recursively copy map[string]any, []any, and primitive values) so nested maps
and slices are duplicated rather than referenced — you can implement a small
helper (e.g., deepCopyAny) that switches on types and recurses, or use a safe
marshal/unmarshal approach, and replace the current loop to call that helper for
each value before inserting into dst.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d6be8ff3-f0da-4742-8046-afbc62ae9d9a
📒 Files selected for processing (7)
internal/admin/dashboard/static/js/modules/live-logs.jsinternal/app/app.gointernal/auditlog/auditlog_test.gointernal/auditlog/logger.gointernal/live/broker.gointernal/usage/logger.gointernal/usage/usage_test.go
Comment on lines
+178
to
+188
| latest := b.events[len(b.events)-1].Seq | ||
| if cursor < latest && latest-cursor > uint64(b.replayLimit) { | ||
| return b.activeSnapshotsLocked(), true | ||
| } | ||
| replay := make([]Event, 0, min(len(b.events), b.replayLimit)) | ||
| for _, event := range b.events { | ||
| if event.Seq > cursor { | ||
| replay = append(replay, event) | ||
| } | ||
| } | ||
| return replay, false |
There was a problem hiding this comment.
When a dashboard tab reconnects with a cursor from before a server restart, the new broker can have a much lower latest sequence. This branch treats cursor > latest as a normal up-to-date stream, returns no replay, and does not send a reset. The tab keeps asking with the stale high cursor, so in-flight audit/usage snapshots from the new process are never replayed after reconnect.
Context Used: CLAUDE.md (source)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/admin/dashboard/static/js/modules/live-logs.js`:
- Around line 35-42: The code in readLiveLogsStream is building a root-relative
URL ('/admin/live/logs') which ignores SERVER_BASE_PATH; change it to construct
the URL via the dashboard's base-path-aware helper used elsewhere (the same
helper the rest of the dashboard uses) instead of a hardcoded string, preserving
the query params built from this.liveLogsLastSeq and requestOptions (and attach
controller.signal as before); do the same fix for the audit/detail fetch
referenced around lines 379-380 so both the SSE/live logs endpoint and the audit
detail fetch use the base-path-aware helper rather than hardcoded '/admin/…'
paths.
In `@internal/app/app.go`:
- Around line 106-113: The live broker is being enabled unconditionally from
appCfg via live.NewBroker and app.live.Enabled(), causing snapshot/marshal costs
even when admin endpoints are disabled or initAdmin(...) fails; instead, compute
an effective "live enabled" flag only after admin initialization succeeds and
then call SetLivePublisher(...) (and any paths that read app.live.Enabled())
conditionally; change the wiring around live.NewBroker/app.live and calls to
SetLivePublisher, moving or gating those calls so they run only when
initAdmin(...) completed successfully (and apply the same conditional gating to
the other occurrences mentioned: the blocks around the uses at the sites
corresponding to lines 130-136, 167-173, and 472-493).
In `@internal/live/broker.go`:
- Around line 170-188: In replayAfterLocked, handle the case where an incoming
cursor is ahead of the broker's latest Seq by treating it as a reset: if cursor
> latest (where latest := b.events[len(b.events)-1].Seq) return
b.activeSnapshotsLocked(), true instead of producing an empty replay; update the
conditional logic around latest/cursor (and consider empty b.events) to detect
this out-of-range cursor and mark Reset=true, and add a regression test that
simulates a restart reconnect with cursor > latest to assert the broker returns
activeSnapshotsLocked() and Reset==true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b1449505-24f4-4c2c-9fe0-55f36d8a9542
📒 Files selected for processing (8)
internal/admin/dashboard/static/js/modules/live-logs.jsinternal/app/app.gointernal/auditlog/auditlog_test.gointernal/auditlog/logger.gointernal/live/broker.gointernal/live/broker_test.gointernal/usage/logger.gointernal/usage/usage_test.go
Comment on lines
+531
to
+560
| if entry.Data != nil { | ||
| preview.ErrorMessage = entry.Data.ErrorMessage | ||
| data := auditPreviewData{ | ||
| WorkflowFeatures: entry.Data.WorkflowFeatures, | ||
| Failover: entry.Data.Failover, | ||
| } | ||
| if auditPreviewIncludesCapturedData(eventType) { | ||
| data.UserAgent = entry.Data.UserAgent | ||
| data.APIKeyHash = entry.Data.APIKeyHash | ||
| data.Temperature = entry.Data.Temperature | ||
| data.MaxTokens = entry.Data.MaxTokens | ||
| data.ErrorMessage = entry.Data.ErrorMessage | ||
| data.ErrorCode = entry.Data.ErrorCode | ||
| data.RequestHeaders = entry.Data.RequestHeaders | ||
| data.ResponseHeaders = entry.Data.ResponseHeaders | ||
| data.RequestBody = entry.Data.RequestBody | ||
| data.ResponseBody = entry.Data.ResponseBody | ||
| data.RequestBodyTooBigToHandle = entry.Data.RequestBodyTooBigToHandle | ||
| data.ResponseBodyTooBigToHandle = entry.Data.ResponseBodyTooBigToHandle | ||
| } | ||
| if data.hasValues() { | ||
| preview.Data = &data | ||
| } | ||
| } | ||
| return preview | ||
| } | ||
|
|
||
| func auditPreviewIncludesCapturedData(eventType string) bool { | ||
| return eventType == EventAuditCompleted || eventType == EventAuditFlushed || eventType == EventAuditFailed | ||
| } |
Contributor
There was a problem hiding this comment.
Keep lifecycle frames compact; don’t embed full audit detail payloads here.
audit.completed and audit.flushed now serialize captured headers/bodies into the live stream. That bypasses the new details-on-demand path in live-logs.js and can send the same large payload twice per request (completed and flushed). Keep these lifecycle events compact and reserve full request/response detail for the explicit detail fetch flow.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/live/broker.go (1)
543-555:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep lifecycle SSE frames compact.
audit.completed/audit.failed/audit.flushedstill embed captured headers and request/response bodies in the live stream. That bypasses the details-on-demand flow described in this PR and can send the same large payload multiple times on the hot SSE path. Keep the live row compact here and fetch full captured detail through the explicit audit-detail path instead.Also applies to: 564-566
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/live/broker.go` around lines 543 - 555, The SSE lifecycle handlers currently embed full captured headers and bodies when auditPreviewIncludesCapturedData(eventType) is true; remove assignment of heavy fields so lifecycle frames remain compact. In the block guarded by auditPreviewIncludesCapturedData(eventType) (and the similar block at the later lines called out), stop copying RequestHeaders, ResponseHeaders, RequestBody, ResponseBody and the large-body flags (RequestBodyTooBigToHandle, ResponseBodyTooBigToHandle) from entry.Data into data; keep only small preview fields like UserAgent, APIKeyHash, Temperature, MaxTokens, ErrorMessage, ErrorCode and fetch full captured details via the audit-detail endpoint instead. Ensure eventType/auditPreviewIncludesCapturedData and the data variable remain for small metadata only.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/app/app.go`:
- Around line 103-112: The App constructor creates app.live via live.NewBroker
(using live.Config) before performing many fallible initializations, and on
early returns those resources are not closed; modify New so that either app.live
is created only after all subsequent initializations succeed or ensure it is
closed on any error path: e.g., if you keep creating app.live upfront, add a
defer-like cleanup that calls app.live.Close() (or the broker's shutdown method)
on any early return, or move the live.NewBroker call to after the fallible steps
so app.live is only allocated on successful startup; update the constructor that
returns (*App, error) and reference app.live, New, and
live.NewBroker/live.Config when applying the change.
In `@internal/live/broker.go`:
- Around line 298-303: The code currently treats EventAuditFailed and
EventUsageFailed as terminal by calling deleteActiveSnapshot (via
deleteActiveSnapshot(b.activeAudit, auditActiveKeys(*event)) and
deleteActiveSnapshot(b.activeUsage, usageActiveKeys(*event))), which prematurely
removes rows from activeSnapshotsLocked(); change the handlers so that *.failed
events do NOT call deleteActiveSnapshot and only EventAuditFlushed,
EventAuditRemoved, and EventUsageFlushed trigger deletion—i.e., remove the
deleteActiveSnapshot call from the EventAuditFailed and EventUsageFailed cases
(and the corresponding duplicate occurrences noted) so rows remain live until
the flush/remove events.
---
Duplicate comments:
In `@internal/live/broker.go`:
- Around line 543-555: The SSE lifecycle handlers currently embed full captured
headers and bodies when auditPreviewIncludesCapturedData(eventType) is true;
remove assignment of heavy fields so lifecycle frames remain compact. In the
block guarded by auditPreviewIncludesCapturedData(eventType) (and the similar
block at the later lines called out), stop copying RequestHeaders,
ResponseHeaders, RequestBody, ResponseBody and the large-body flags
(RequestBodyTooBigToHandle, ResponseBodyTooBigToHandle) from entry.Data into
data; keep only small preview fields like UserAgent, APIKeyHash, Temperature,
MaxTokens, ErrorMessage, ErrorCode and fetch full captured details via the
audit-detail endpoint instead. Ensure eventType/auditPreviewIncludesCapturedData
and the data variable remain for small metadata only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4f898e14-3c74-48f4-947f-0b32eb3e93d6
📒 Files selected for processing (9)
internal/admin/dashboard/static/js/modules/live-logs.jsinternal/admin/dashboard/static/js/modules/live-logs.test.cjsinternal/app/app.gointernal/auditlog/auditlog_test.gointernal/auditlog/logger.gointernal/live/broker.gointernal/live/broker_test.gointernal/usage/logger.gointernal/usage/usage_test.go
Comment on lines
103
to
112
| app := &App{ | ||
| config: appCfg, | ||
| } | ||
| app.live = live.NewBroker(live.Config{ | ||
| Enabled: appCfg.Admin.LiveLogsEnabled, | ||
| BufferSize: appCfg.Admin.LiveLogsBufferSize, | ||
| ReplayLimit: appCfg.Admin.LiveLogsReplayLimit, | ||
| Heartbeat: time.Duration(appCfg.Admin.LiveLogsHeartbeatSeconds) * time.Second, | ||
| }) | ||
|
|
Contributor
There was a problem hiding this comment.
Close the live broker on constructor error paths.
At Line 106, app.live is created before a long chain of fallible initializations, but early return nil, ... branches in New do not close it. If live logs are enabled, failed startup attempts can leak broker resources (e.g., goroutines/timers).
♻️ Suggested minimal fix
app := &App{
config: appCfg,
}
app.live = live.NewBroker(live.Config{
Enabled: appCfg.Admin.LiveLogsEnabled,
BufferSize: appCfg.Admin.LiveLogsBufferSize,
ReplayLimit: appCfg.Admin.LiveLogsReplayLimit,
Heartbeat: time.Duration(appCfg.Admin.LiveLogsHeartbeatSeconds) * time.Second,
})
+ liveNeedsClose := true
+ defer func() {
+ if liveNeedsClose && app.live != nil {
+ app.live.Close()
+ }
+ }()
...
if livePublishersEnabled {
app.attachLivePublishers()
}
app.server = server.New(provider, serverCfg)
+ liveNeedsClose = false
return app, nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/app/app.go` around lines 103 - 112, The App constructor creates
app.live via live.NewBroker (using live.Config) before performing many fallible
initializations, and on early returns those resources are not closed; modify New
so that either app.live is created only after all subsequent initializations
succeed or ensure it is closed on any error path: e.g., if you keep creating
app.live upfront, add a defer-like cleanup that calls app.live.Close() (or the
broker's shutdown method) on any early return, or move the live.NewBroker call
to after the fallible steps so app.live is only allocated on successful startup;
update the constructor that returns (*App, error) and reference app.live, New,
and live.NewBroker/live.Config when applying the change.
Comment on lines
+298
to
+303
| case EventAuditFailed, EventAuditFlushed, EventAuditRemoved: | ||
| deleteActiveSnapshot(b.activeAudit, auditActiveKeys(*event)) | ||
| return | ||
| case EventUsageFailed, EventUsageFlushed: | ||
| deleteActiveSnapshot(b.activeUsage, usageActiveKeys(*event)) | ||
| return |
Contributor
There was a problem hiding this comment.
Keep *.failed rows live until *.flushed.
audit.failed / usage.failed are treated as terminal here, which contradicts the PR behavior to mark rows as persisted only after flush events. A reconnect between *.failed and *.flushed will drop the row from activeSnapshotsLocked(), and audit rows flip _live_pending to false too early.
💡 Minimal fix
- case EventAuditFailed, EventAuditFlushed, EventAuditRemoved:
+ case EventAuditFlushed, EventAuditRemoved:
deleteActiveSnapshot(b.activeAudit, auditActiveKeys(*event))
return
- case EventUsageFailed, EventUsageFlushed:
+ case EventUsageFlushed:
deleteActiveSnapshot(b.activeUsage, usageActiveKeys(*event))
return
@@
-func auditEventTerminal(eventType string) bool {
- return eventType == EventAuditFailed || eventType == EventAuditFlushed || eventType == EventAuditRemoved
+func auditEventTerminal(eventType string) bool {
+ return eventType == EventAuditFlushed || eventType == EventAuditRemoved
}Also applies to: 526-527, 585-587
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/live/broker.go` around lines 298 - 303, The code currently treats
EventAuditFailed and EventUsageFailed as terminal by calling
deleteActiveSnapshot (via deleteActiveSnapshot(b.activeAudit,
auditActiveKeys(*event)) and deleteActiveSnapshot(b.activeUsage,
usageActiveKeys(*event))), which prematurely removes rows from
activeSnapshotsLocked(); change the handlers so that *.failed events do NOT call
deleteActiveSnapshot and only EventAuditFlushed, EventAuditRemoved, and
EventUsageFlushed trigger deletion—i.e., remove the deleteActiveSnapshot call
from the EventAuditFailed and EventUsageFailed cases (and the corresponding
duplicate occurrences noted) so rows remain live until the flush/remove events.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tests
Summary by CodeRabbit
New Features
Configuration
Documentation
Tests