-
-
Couldn't load subscription status.
- Fork 4.5k
feat(logs): Add better loading state for flex time strategy #102104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(logs): Add better loading state for flex time strategy #102104
Conversation
Previously, the flex time strategy could result in a really long loading state. This change shows a live progress with the bytes scanned in the loading state to indicate we're still working on it.
| setTotalBytesScanned(0); | ||
| return; | ||
| } | ||
|
|
||
| const bytesScanned = lastPage[0].meta?.bytesScanned; | ||
| if (!defined(bytesScanned)) { | ||
| return; | ||
| } | ||
|
|
||
| setTotalBytesScanned(previousBytesScanned => previousBytesScanned + bytesScanned); | ||
| }, [lastPage]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: The useEffect accumulates bytesScanned multiple times for the same page due to lastPage reference changes on React Query state updates.
Severity: CRITICAL | Confidence: 1.00
🔍 Detailed Analysis
The useEffect hook responsible for accumulating bytesScanned re-runs unnecessarily because its dependency, lastPage, receives a new object reference on every React Query state update, even if the underlying page data has not changed. This causes the bytesScanned value from the same page to be added multiple times to totalBytesScanned, leading to an inflated and incorrect total displayed to the user. This occurs whenever React Query updates its internal state, such as when isPending changes, triggering the effect with a new lastPage reference that points to the same data.
💡 Suggested Fix
Modify the useEffect to ensure bytesScanned is added only once per unique lastPage data object. This can be achieved by tracking a stable identifier from lastPage (e.g., lastPage.id or a unique hash of its content) and only performing the accumulation if this identifier has not been processed before.
🤖 Prompt for AI Agent
Fix this bug. In static/app/views/explore/logs/useLogsQuery.tsx at lines 587-606: The
`useEffect` hook responsible for accumulating `bytesScanned` re-runs unnecessarily
because its dependency, `lastPage`, receives a new object reference on every React Query
state update, even if the underlying page data has not changed. This causes the
`bytesScanned` value from the same page to be added multiple times to
`totalBytesScanned`, leading to an inflated and incorrect total displayed to the user.
This occurs whenever React Query updates its internal state, such as when `isPending`
changes, triggering the effect with a new `lastPage` reference that points to the same
data.
Did we get this right? 👍 / 👎 to inform future reviews.
Previously, the flex time strategy could result in a really long loading state. This change shows a live progress with the bytes scanned in the loading state to indicate we're still working on it. Closes LOGS-438
Previously, the flex time strategy could result in a really long loading state. This change shows a live progress with the bytes scanned in the loading state to indicate we're still working on it.
Closes LOGS-438