feat(webui): Display total result count rather than UI table row count for Presto queries. #1236
Conversation
WalkthroughAdds client support for a server-provided total-results metadata count: new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant UI as SearchPage
participant HK as useUpdateStateWithMetadata
participant ST as Search Store
participant SV as Server
U->>UI: Start search
UI->>SV: Submit search
SV-->>UI: Stream rows + resultsMetadata(numTotalResults)
UI->>HK: Hook observes signal/metadata
HK->>ST: updateNumSearchResultsMetadata(numTotalResults)
Note right of ST #DDEBF7: Store holds table/timeline/metadata counts
HK->>UI: Dispatch DONE/FAILED UI state
UI->>UI: Results.tsx computes numResults = max(metadata, timeline, table)
UI-->>U: Render status and counts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (2)📚 Learning: 2025-06-09T17:15:30.309ZApplied to files:
📚 Learning: 2025-07-18T20:00:50.288ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (5)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
components/webui/client/src/pages/SearchPage/SearchQueryStatus/index.tsx (1)
27-32: Missing space after label and inconsistent capitalisation (“ID”)Currently renders as “Search job id:123 found”. Add a space after the colon and capitalise “ID”.
Apply this diff:
- Search job id: + Search job ID:{" "} {searchJobId} {" "} found {" "}components/webui/client/src/pages/SearchPage/SearchQueryStatus/Results.tsx (2)
22-38: Guard against invalid/negative counts and stale metadata dominance
- Consider clamping at 0 to handle defensive cases (e.g., transient negatives) and avoid showing “-1”.
- Because max() includes metadata, stale large metadata from a previous query can dominate until new metadata arrives. Resetting the metadata count at the start of a new results set in useUpdateStateWithMetadata.ts (see separate comment) will prevent this.
Apply this diff:
- const numResults = useMemo( - () => Math.max(numSearchResultsMetadata, numSearchResultsTimeline, numSearchResultsTable), + const numResults = useMemo( + () => Math.max( + 0, + numSearchResultsMetadata, + numSearchResultsTimeline, + numSearchResultsTable + ), [ numSearchResultsMetadata, numSearchResultsTimeline, numSearchResultsTable, ] );
31-38: useMemo isn’t buying much hereComputing Math.max of three numbers is trivial; useMemo adds complexity with negligible benefit. Consider inlining numResults without memoisation.
Apply this diff:
- const numResults = useMemo( - () => Math.max( - 0, - numSearchResultsMetadata, - numSearchResultsTimeline, - numSearchResultsTable - ), - [ - numSearchResultsMetadata, - numSearchResultsTimeline, - numSearchResultsTable, - ] - ); + const numResults = Math.max( + 0, + numSearchResultsMetadata, + numSearchResultsTimeline, + numSearchResultsTable + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
components/webui/client/src/pages/SearchPage/SearchQueryStatus/Results.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchQueryStatus/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchState/index.tsx(4 hunks)components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts(2 hunks)components/webui/client/src/pages/SearchPage/index.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/client/src/pages/SearchPage/SearchQueryStatus/Results.tsxcomponents/webui/client/src/pages/SearchPage/SearchQueryStatus/index.tsxcomponents/webui/client/src/pages/SearchPage/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.tscomponents/webui/client/src/pages/SearchPage/SearchState/index.tsx
🔇 Additional comments (7)
components/webui/client/src/pages/SearchPage/SearchState/index.tsx (3)
47-51: Good addition and clear documentationAdding numSearchResultsMetadata with a concise description aligns well with the PR objective.
106-106: Updater naming and symmetry LGTMThe updater name matches existing patterns (table/timeline) and reads clearly in downstream code.
127-129: Setter implementation is correct; consider guarding against NaN upstreamThe setter is fine. Upstream, ensure only finite, non-negative numbers flow in (see suggestions in useUpdateStateWithMetadata.ts) so UI doesn’t momentarily show “NaN” or negative counts if the backend sends unexpected values.
components/webui/client/src/pages/SearchPage/index.tsx (1)
11-21: Hook rename integration verifiedThe named export in
useUpdateStateWithMetadata.tsmatches the import inSearchPage/index.tsx. No further action needed.• components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts (line 61):
export { useUpdateStateWithMetadata };components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts (3)
15-20: Docstring aligns with behaviour changeAccurately describes UI state transitions and the new metadata-driven count update.
55-58: Dependency array is correct; no extra deps neededIncluding updateNumSearchResultsMetadata is necessary. The ref does not need to be added.
61-62: Export name matches usageThe named export aligns with the new import in SearchPage.
| const SEARCH_STATE_DEFAULT = Object.freeze({ | ||
| aggregationJobId: null, | ||
| cachedDataset: null, | ||
| numSearchResultsMetadata: 0, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider resetting metadata count at query start to avoid stale max() values
Initialising to 0 is fine, but if a new query begins and the first metadata payload lacks numTotalResults, the previous query’s (possibly large) metadata count can linger and dominate the max(metadata, timeline, table) in Results.tsx. Ensure this field is explicitly reset to 0 on new query start or when a new results set (_id) is detected.
Run this to find where the metadata count is reset on query start and whether a 0 reset exists:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# All call sites of updateNumSearchResultsMetadata
rg -nP -C2 --type=ts --type=tsx '\bupdateNumSearchResultsMetadata\s*\('
# Explicit resets to 0
rg -nP -C3 --type=ts --type=tsx 'updateNumSearchResultsMetadata\s*\(\s*0\s*\)'Length of output: 121
I’ll retry the grep without relying on --type=tsx support:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all call sites of updateNumSearchResultsMetadata in .ts and .tsx files
rg -nP -C2 '\bupdateNumSearchResultsMetadata\s*\(' -g '*.ts' -g '*.tsx'
# Find any explicit resets to 0
rg -nP -C3 'updateNumSearchResultsMetadata\s*\(\s*0\s*\)' -g '*.ts' -g '*.tsx'Length of output: 783
Ensure metadata count is reset on every new query
The handler updateNumSearchResultsMetadata is only invoked when resultsMetadata.numTotalResults is defined—there’s no explicit reset to 0 when a query starts or when a new results set (_id) arrives. As a result, if the first payload of a new query omits numTotalResults, the previous (larger) value will persist and skew any max() calculations in Results.tsx.
Please add an explicit reset to 0 in useUpdateStateWithMetadata.ts, for example:
• File: components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts
useEffect(() => {
// Reset metadata count on each new query (or when _id changes)
updateNumSearchResultsMetadata(0);
if (typeof resultsMetadata.numTotalResults !== "undefined") {
updateNumSearchResultsMetadata(resultsMetadata.numTotalResults);
}
}, [resultsMetadata._id /* or the relevant query identifier */]);This guarantees stale values are cleared before processing each new result set.
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts
around where resultsMetadata is handled (related to lines near the current
numSearchResultsMetadata usage), the metadata counter is never explicitly reset
when a new query or results set (_id) begins, allowing stale larger values to
persist; modify the effect that reads resultsMetadata so it first calls
updateNumSearchResultsMetadata(0) to clear the count, then only sets the real
value if resultsMetadata.numTotalResults is defined, and ensure the effect
depends on the query identifier (e.g., resultsMetadata._id) so the reset runs
for each new query.
| const useUpdateStateWithMetadata = () => { | ||
| const {updateSearchUiState, updateNumSearchResultsMetadata} = useSearchStore(); | ||
| const resultsMetadata = useResultsMetadata(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Select store actions with selectors to avoid re-rendering on every state change
Destructuring the whole store subscribes to all state changes, causing SearchPage to re-render frequently. Select only the actions needed.
Apply this diff:
-const useUpdateStateWithMetadata = () => {
- const {updateSearchUiState, updateNumSearchResultsMetadata} = useSearchStore();
+const useUpdateStateWithMetadata = () => {
+ const updateSearchUiState = useSearchStore((s) => s.updateSearchUiState);
+ const updateNumSearchResultsMetadata = useSearchStore((s) => s.updateNumSearchResultsMetadata);
const resultsMetadata = useResultsMetadata();🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts
around lines 21 to 24, the current destructuring of useSearchStore subscribes to
the whole store and causes unnecessary re-renders; instead, select only the
specific actions you need from the store (e.g. use the selector form of
useSearchStore to pick updateSearchUiState and updateNumSearchResultsMetadata,
or call useSearchStore twice to read each action separately) so the component
only subscribes to those actions; ensure the selected action references are
stable (wrap callbacks where needed) and remove the current broad destructuring.
components/webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts
Show resolved
Hide resolved
|
im still thinking about how to do that. Im not sure if we should do what was suggested, or just a icon with tooltip. Or just render some message at the bottom of the table. Either way it will need to be conditionally rendered so may be much for this PR? Anyways we probably shouldnt merge this PR until after release, to prevent issues with release |
components/webui/client/src/pages/SearchPage/SearchQueryStatus/index.tsx
Outdated
Show resolved
Hide resolved
…/index.tsx Co-authored-by: hoophalab <200652805+hoophalab@users.noreply.github.com>
There was a problem hiding this comment.
Deferring to @hoophalab's review.
For the PR title, how about:
feat(webui): Display total result count rather than UI table row count for Presto queries.
hoophalab
changed the title
4 participants
Description
Previous result count only considered the number of results in the table for presto queries. This PR allows the UI to also include the result count from server metadata. After this PR, the result count is no longer limited to 1k results, and will show the true amount of results from presto in query status header. The table is still limited to 1k results.
Checklist
breaking change.
Validation performed
Ran long query and verified results >1000 in header
Summary by CodeRabbit
New Features
Style