Skip to content

fix(logs) - Fix logs table when auto-refresh toggles#116332

Open
adrianviquez wants to merge 3 commits into
masterfrom
adrian/fix-logs-resolve-table-inconsistent-results-when-refreshing
Open

fix(logs) - Fix logs table when auto-refresh toggles#116332
adrianviquez wants to merge 3 commits into
masterfrom
adrian/fix-logs-resolve-table-inconsistent-results-when-refreshing

Conversation

@adrianviquez
Copy link
Copy Markdown
Contributor

@adrianviquez adrianviquez commented May 27, 2026

The logs table behaves inconsistently in two scenarios:

  1. When you toggle the auto-refresh button and then click the manual refresh button (behavior seen more clearly in low traffic organizations)
  2. When you toggle the auto-refresh button, on and off, and then click the manual refresh button

See this video

The fixes:

  1. Disable the manual refresh button while the auto-refresh is on. It feels redundant to have the manual button accessible from a UX perspective when auto-referesh is on, therefore I added logic to disable it and a tooltip explaining why.
  2. Add logic to reset the query when toggle goes from on to off + set the virtual timestamp to undefined
  • This line is used to dictate if we show all rows, and the virtual timestamp's value wasn't being changed when untoggling - the bug
  • Resetting the query marks the cache data as stale and triggers a refetch with the latest, non-virtual data.

Result:

Additionally, added tests and modified the virtualStreaming spec to use the location router instead of the mocked one.

Closes LOGS-821

@adrianviquez adrianviquez requested a review from a team as a code owner May 27, 2026 19:19
@linear-code
Copy link
Copy Markdown

linear-code Bot commented May 27, 2026

LOGS-821

@github-actions github-actions Bot added the Scope: Frontend Automatically applied to PRs that change frontend components label May 27, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 27, 2026

📊 Type Coverage Diff

✅ No new type safety issues introduced. Coverage: 93.59%

@adrianviquez
Copy link
Copy Markdown
Contributor Author

@cursor review

Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 483c2c2. Configure here.

Copy link
Copy Markdown
Contributor

@nsdeschenes nsdeschenes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small nit, and a question 👀

Comment on lines +363 to +365
<ProviderWrapper>
<LogsTabContentHarness datePageFilterProps={datePageFilterProps} />
</ProviderWrapper>,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: You can just pass ProviderWrapper to the additionalWrapper field in the second argument object.

Comment on lines +295 to +300
// Reset the query data when auto-refresh is disabled to avoid stale data when switching between modes
useEffect(() => {
if (prevAutorefreshEnabled && !autorefreshEnabled) {
queryClient.resetQueries({queryKey: tableData.queryKey});
}
}, [autorefreshEnabled, prevAutorefreshEnabled, queryClient, tableData.queryKey]);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anyway to move this logic into the event handler the user is using to toggle auto-refresh?


const [sidebarOpen, setSidebarOpen] = useState(mode === Mode.AGGREGATE);

const prevAutorefreshEnabled = usePrevious(autorefreshEnabled);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we shouldn’t use the usePrevious hook, it violates the rules of react and should be deprecated imo. To access information from previous render, see:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants