fix(dashboard): stop live session-thread counts inflating on first-session requests - #608
Conversation
…ssion requests With "Group by session" on, a live row displaced out of the head list (two rows of a fresh session collapsing into one thread, or a new live request replacing the head) was dropped when the thread's children were never fetched. Every later live event for that request then matched nothing on screen and was re-counted as a new session member, bumping the thread badge +1 per event — 2x-3x inflation whenever a session's first messages rendered live. Displaced entries are now retained in a partial children list (loaded: false), so late events merge in place; expanding still triggers the full fetch, which preserves not-yet-persisted live entries on top of the server page. Also folds the duplicated audit.detail branch of mergeLiveAuditEntry into the main flow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAudit-log thread handling now preserves live displaced entries during fetches, avoids duplicate expansions, separates detail-event processing, and maintains accurate child and session totals. Tests cover head filtering, live retention, interleaved session requests, and lifecycle updates. ChangesAudit log thread merging
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant LiveEvent
participant LiveLogMerger
participant ThreadChildren
participant AuditList
participant AuditAPI
LiveEvent->>LiveLogMerger: merge audit event
LiveLogMerger->>ThreadChildren: retain or prepend displaced child
AuditList->>AuditAPI: fetch thread entries
AuditAPI-->>AuditList: fetched entries and total
AuditList->>ThreadChildren: merge fetched and preserved entries
ThreadChildren-->>AuditList: loaded entries and adjusted total
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@web/dashboard/src/pages/audit-logs/auditList.svelte.js`:
- Around line 228-247: Update the restore callback and related toggleThread
state handling so deleting an empty auditThreadChildren[sessionId] also clears
or marks auditExpandedThreads[sessionId] for retry. Ensure the next toggle after
a failed or stale empty fetch invokes fetchThreadEntries immediately rather than
being interpreted as a collapse.
- Around line 262-271: Update the merge call in fetchThreadEntries so the old
in-flight head is not included in the headKeys used by mergeAuditThreadChildren;
pass only the currently valid head and currentHead as appropriate, preserving
the demoted head as a live child. Add a regression test covering a
foldLiveAuditIntoThread/regroupLiveAuditHead head replacement while
fetchThreadEntries is in flight, verifying the displaced child remains in the
merged results.
🪄 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 Plus
Run ID: 0d8b9346-e42a-4cc5-8b48-7d3316b651f9
⛔ Files ignored due to path filters (2)
internal/admin/dashboard/static/dist/assets/index-BmAzcbJM.jsis excluded by!**/dist/**internal/admin/dashboard/static/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (5)
web/dashboard/src/pages/audit-logs/audit-logic.jsweb/dashboard/src/pages/audit-logs/auditList.svelte.jsweb/dashboard/src/pages/audit-logs/live-logs-logic.jsweb/dashboard/tests/audit-list.test.jsweb/dashboard/tests/live-logs.test.js
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Confidence Score: 5/5The PR appears safe to merge. The current implementation excludes only the replacement head when an in-flight session fetch completes, allowing the displaced original head to remain in the merged child list; no blocking failure remains.
 T-Rex LogsWhat T-Rex did
Reviews (2): Last reviewed commit: "fix(dashboard): keep mid-flight demoted ..." | Re-trigger Greptile |
…retry collapsed failed fetches Review follow-ups: exclude only the CURRENT head when merging a fetched session page (a head replaced during the fetch is a demoted child the page must keep), and collapse an expanded thread when a failed/stale fetch leaves nothing to show so a single click retries instead of three. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed both review findings in d142d30: Head replacement mid-flight (CodeRabbit major / Greptile P1) — confirmed, fixed as proposed. Excluding the original Failed/stale fetch leaves a dead expanded thread (CodeRabbit minor) — valid, pre-existing before this PR, fixed. When the restore path drops an empty slot, the thread is now also collapsed, so a single click retries instead of three. When live-displaced entries remain, the thread stays expanded showing them, and the next expand still refetches ( |
Problem
With "Group by session" enabled and the Audit Logs page open, sending the first messages of a fresh session made the thread count badge (
audit-thread-count) read 2x–3x the real number of requests. Refreshing the page fixed it, and later messages appended to an existing session counted correctly.Root cause
audit.startedlive events are published before session detection stamps the request context, so every live row is first inserted sessionless and only later events carry thesession_id. When a later event made two on-screen rows collapse into one thread (or a new live request displaced a fetched head), the demoted row was dropped if the thread's children list had never been lazily fetched —prependLiveAuditThreadChildwas a no-op for unloaded threads.Every subsequent live event for that dropped request (updated, stream chunks, completed, flushed) then matched nothing on screen, so
foldLiveAuditIntoThreadre-counted it as a brand-new session member — +1 on the badge per event. The bug only shows on a session's first live-rendered messages because that is the only time several rows of one session are live with events still in flight.Fix
prependLiveAuditThreadChildnow always retains the displaced entry: for never-expanded threads it creates a partial children list markedloaded: false. Late events find their entry and merge in place instead of re-counting.toggleThreadtreats partial lists as unfetched (checksloaded/loadingrather than slot existence), so expanding still fetches the full session page.fetchThreadEntrieskeeps live-displaced entries visible while the page loads and merges them on top of the fetched page via the newmergeAuditThreadChildrenhelper (deduped by id/request_id, including against a head replaced mid-flight; still-unpersisted live entries are preserved and counted into the thread total). Failed/stale fetches keep the partial entries instead of deleting the slot.Refactor (while there)
mergeLiveAuditEntrycarried a near-complete duplicate of its merge flow foraudit.detailevents; it is now a single flow with anisDetailflag (detail events still never re-trigger the detail fetch, and never fold), with lifecycle-state stamping extracted intoliveAuditPatch.auditThreadChildEntriesgrew intomergeAuditThreadChildrenand the old helper was removed.User-visible impact
The thread badge now equals the number of distinct requests in the session while watching live traffic. Transient approximations remain only in genuinely unknowable cases (e.g. events replayed after a thread scrolled off the page) and reconcile on the next
/admin/audit/sessionsrefetch, which was always correct.Tests
audit.started, late session ids, completed/flushed for both): badge stays at 2 — the old code produced 4.svelte-checkclean, dist rebuilt.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests