feat: Auto refresh Query Log - #8487
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an optional “live” (auto-refreshing) mode to the Query Log UI, with a user-configurable refresh interval persisted in localStorage and surfaced in Logs settings.
Changes:
- Introduces an auto-refresh toggle next to the Query Log refresh button, backed by localStorage.
- Adds a Logs settings section to choose the auto-refresh interval.
- Adds constants, styling, and i18n strings to support the new UI.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| client/src/helpers/localStorageHelper.ts | Adds new localStorage keys for auto-refresh enabled/interval settings. |
| client/src/helpers/constants.ts | Defines allowed auto-refresh intervals and a default interval constant. |
| client/src/components/Settings/LogsConfig/Form.tsx | Inserts the new auto-refresh interval selector into Logs configuration UI. |
| client/src/components/Settings/LogsConfig/AutoRefreshInterval.tsx | New settings component for selecting/storing the refresh interval. |
| client/src/components/Logs/Logs.css | Styles the auto-refresh toggle in the Logs header. |
| client/src/components/Logs/Filters/index.tsx | Adds concurrency guard + “silent” refresh path and renders the auto-refresh toggle. |
| client/src/components/Logs/Filters/AutoRefresh.tsx | New toggle component that schedules periodic silent refreshes. |
| client/src/__locales/en.json | Adds English translations for auto-refresh UI labels. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sil3ntVip3r
left a comment
There was a problem hiding this comment.
The refresh merge treats time as a unique entry ID, but it is only the request start timestamp. The backend assigns it with time.Now(), and the query-log tests explicitly account for low-resolution Windows clocks, so distinct records can share the same serialized time. Exact trigger: the table already contains query A at time T, then a distinct query B is recorded at T; the next response contains B and A, but previousTimes.has(T) rejects both refreshed records and the merge restores only previous A. B remains silently absent on subsequent refreshes and can fall out of the fetched window entirely. Please deduplicate with a stable per-entry ID; absent an API ID, the smallest frontend fix is a collision-resistant identity/overlap using immutable entry fields and occurrence counts rather than timestamp alone. Add a reducer/action test with previous A and refreshed [B, A] sharing T but differing in domain or client, asserting both appear exactly once.
Thanks for the real issue, I will address this! But bro DDoSing the maintainers with 45 PRs at once is crazy |
|
Fair point—I front-loaded too many submissions at once. I’ve paused new PRs and am focusing on existing review feedback and CI so the maintainers aren’t flooded. Thanks for taking the timestamp-collision fix. Once you push it, I’ll limit the follow-up to rechecking the changed merge logic and its regression coverage. |
Sil3ntVip3r
left a comment
There was a problem hiding this comment.
Thanks for fixing the timestamp-only collision. I rechecked exact head
516b962b9b6a551e34571b9f4351cbc6b01e51c5. Two correctness gaps remain.
-
getLogIdentityKeyinclient/src/helpers/helpers.tsx:137-138still omits
immutable outcome fields such asreason,response, rule/filter data,
service name, and original response. Two same-time records with identical
domain, type, client, upstream, status, and cache state but different
outcomes therefore consume the same occurrence count. A deterministic
reproduction with previousAand refreshed[B, A], where onlyreason
andresponsediffer, returns[A, A]and silently losesB.Please build the identity from all immutable normalized record fields while
excluding derived or volatile fields such as tracker metadata. Add the same
reason/responsecollision as a regression test. -
Refresh can leave a permanent pagination gap.
getQueryLogcaps every
request at 20 records (client/src/api/Api.ts:601-605), the short-poll path
does not followoldestwhen the first page is full, and
refreshFilteredLogspreserves the old cursor when more than 20 records
were already loaded (client/src/actions/queryLogs.ts:158-173). Exact
trigger: the UI has 40 old records loaded and 21 new records arrive before
the next refresh. Refresh retrieves the newest 20, does not fetch another
page, and retains the cursor below the 40 old records. The twenty-first new
record lies between those ranges and is skipped by later pagination.Please continue refresh pagination until it reaches a stable overlap with
the previously loaded newest range, then retain a cursor that cannot jump
over unseen records. Add an action-level regression with 40 previous
records and 21 arrivals, asserting all 21 appear exactly once. Use
cancellation and a bounded request/scan budget so a missing overlap cannot
create an unbounded refresh chain.
The existing exact-head helper tests, lint, and type check pass; these are
missing boundary cases rather than existing gate failures. A disposable
helper regression for the first case failed against this head as described and
was removed afterward. The GitHub build and lint runs are action_required
with zero jobs, so they are fork-approval gates, not test failures.
Fixes #666
Alters the refresh button to allow toggling auto-refreshing. Also adds a space in the config for changing this value too.