feat(webui): Remove timeline for Presto queries. #1251
Conversation
WalkthroughSearchPage now conditionally renders Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant SP as SearchPage
participant CFG as Settings
participant C as SearchControls
participant S as SearchQueryStatus
participant T as SearchResultsTable
participant TL as SearchResultsTimeline
U->>SP: Open/Search
SP->>CFG: Read SETTINGS_QUERY_ENGINE
CFG-->>SP: engine value
rect rgba(220,235,255,0.5)
note right of SP: Conditional render
alt engine != PRESTO
SP->>TL: Render Timeline
else engine == PRESTO
SP-->>TL: Skip Timeline
end
end
SP->>C: Render Controls
SP->>S: Render Status
SP->>T: Render Results Table
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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. ✨ 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/webui/client/src/pages/SearchPage/index.tsx (1)
24-31: Reduce duplication by derivingisPrestoonceYou’re checking the engine in two places (progress bar and timeline). Deriving a single boolean improves readability and future edits.
Example:
const isPresto = SETTINGS_QUERY_ENGINE === CLP_QUERY_ENGINES.PRESTO; // ... {isPresto && <ProgressBar/>} {false == isPresto && <SearchResultsTimeline/>} // follows the project’s guideline on negation style
📜 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 (1)
components/webui/client/src/pages/SearchPage/index.tsx(1 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/index.tsx
🔇 Additional comments (2)
components/webui/client/src/pages/SearchPage/index.tsx (2)
30-30: Timeline correctly gated off for Presto — aligns with PR objectiveConditionally rendering the timeline only when the engine is not PRESTO matches the PR’s intent. Looks good.
30-31: No module-level side effects detected in SearchResultsTimelineI’ve inspected
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTimeline/index.tsxand confirmed there are no import-time side effects:
- No network calls (
fetch,axios,XMLHttpRequest) at the module top level- No subscriptions or event listeners (
WebSocket,addEventListener) executed on import- No immediate-execution blocks (IIFEs) outside of React hooks/components
The conditional render in
index.tsxis sufficient; the module itself is side-effect-free.
| {SETTINGS_QUERY_ENGINE !== CLP_QUERY_ENGINES.PRESTO && <SearchResultsTimeline/>} | ||
| <SearchResultsTable/> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Code‑split Timeline to avoid shipping unused code and import‑time side effects
Because SearchResultsTimeline is still imported at the top level, its code is bundled and any module‑level side effects (if any) would still run in PRESTO builds. Consider lazy‑loading the component and wrapping it in Suspense, which also reduces the PRESTO bundle.
Apply this diff within the selected range:
-{SETTINGS_QUERY_ENGINE !== CLP_QUERY_ENGINES.PRESTO && <SearchResultsTimeline/>}
+{SETTINGS_QUERY_ENGINE !== CLP_QUERY_ENGINES.PRESTO && (
+ <Suspense fallback={null}>
+ <SearchResultsTimeline/>
+ </Suspense>
+)}Then, elsewhere in this file:
- Remove the static import on Line 10.
- Add:
import {lazy, Suspense} from "react";
const SearchResultsTimeline = lazy(() => import("./SearchResults/SearchResultsTimeline"));🤖 Prompt for AI Agents
In components/webui/client/src/pages/SearchPage/index.tsx around lines 30-31,
the SearchResultsTimeline is currently statically imported and rendered which
ships its code and possible module side-effects into PRESTO builds; remove the
static import at line 10 and instead import lazy and Suspense from react and
declare SearchResultsTimeline via lazy(() =>
import("./SearchResults/SearchResultsTimeline")); then replace the JSX at lines
30-31 so the timeline is only rendered when SETTINGS_QUERY_ENGINE !==
CLP_QUERY_ENGINES.PRESTO and wrapped in <Suspense fallback={null}> (or another
minimal fallback) to ensure the component is code-split and only loaded when
needed.
hoophalab
left a comment
There was a problem hiding this comment.
Looks good. The timeline is gone in Presto.
kirkrodrigues
left a comment
There was a problem hiding this comment.
Deferring to @hoophalab's review.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
components/webui/client/src/pages/SearchPage/index.tsx (2)
30-30: Add a TODO referencing “Phase II” to clarify intent and revisit timingThis helps future contributors understand why the timeline is gated and when to re-enable it.
Apply this diff in place:
- {SETTINGS_QUERY_ENGINE !== CLP_QUERY_ENGINES.PRESTO && <SearchResultsTimeline/>} + // TODO(Phase II - Presto UI): Enable timeline for PRESTO once supported. + {SETTINGS_QUERY_ENGINE !== CLP_QUERY_ENGINES.PRESTO && <SearchResultsTimeline/>}
30-30: Code‑split Timeline to avoid bundling unused code and import-time side effectsThe static import at Line 10 ships timeline code (and any module-level side effects) even when PRESTO hides it. Lazy-load to keep PRESTO bundles lean and defer work.
Within this line range, wrap the conditional render with Suspense:
- {SETTINGS_QUERY_ENGINE !== CLP_QUERY_ENGINES.PRESTO && <SearchResultsTimeline/>} + {false == (SETTINGS_QUERY_ENGINE === CLP_QUERY_ENGINES.PRESTO) && ( + <Suspense fallback={null}> + <SearchResultsTimeline/> + </Suspense> + )}And elsewhere in this file (imports section), replace the static import with a lazy one:
-import SearchResultsTimeline from "./SearchResults/SearchResultsTimeline"; +import {lazy, Suspense} from "react"; +const SearchResultsTimeline = lazy(() => import("./SearchResults/SearchResultsTimeline"));
📜 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 (1)
components/webui/client/src/pages/SearchPage/index.tsx(1 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/index.tsx
🧬 Code graph analysis (1)
components/webui/client/src/pages/SearchPage/index.tsx (1)
components/webui/client/src/config/index.ts (1)
SETTINGS_QUERY_ENGINE(26-26)
⏰ 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). (1)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (1)
components/webui/client/src/pages/SearchPage/index.tsx (1)
30-30: Timeline correctly gated for PRESTO — aligns with the PR objectiveRendering the timeline only when the engine is not PRESTO meets the stated goal to remove timeline for Presto queries.
| <SearchQueryStatus/> | ||
| </div> | ||
| <SearchResultsTimeline/> | ||
| {SETTINGS_QUERY_ENGINE !== CLP_QUERY_ENGINES.PRESTO && <SearchResultsTimeline/>} |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add a focused test to lock in the PRESTO gating behaviour
A small test guards against regressions: timeline not rendered for PRESTO; rendered otherwise.
Example (React Testing Library):
import {render, screen} from "@testing-library/react";
import * as config from "../../config";
import SearchPage from "./index";
test("does not render timeline for PRESTO", () => {
jest.spyOn(config, "SETTINGS_QUERY_ENGINE", "get").mockReturnValue(config.CLP_QUERY_ENGINES.PRESTO);
render(<SearchPage/>);
expect(screen.queryByTestId("search-results-timeline")).toBeNull();
});
test("renders timeline for non-PRESTO", () => {
jest.spyOn(config, "SETTINGS_QUERY_ENGINE", "get").mockReturnValue(config.CLP_QUERY_ENGINES.CLP); // or another engine
render(<SearchPage/>);
expect(screen.getByTestId("search-results-timeline")).toBeInTheDocument();
});Note: Add data-testid="search-results-timeline" to the root element of SearchResultsTimeline if not already present.
🤖 Prompt for AI Agents
In components/webui/client/src/pages/SearchPage/index.tsx around line 30, the
conditional rendering for PRESTO needs a focused test and the Timeline component
should expose a test id; add data-testid="search-results-timeline" to the root
element of SearchResultsTimeline, then add two React Testing Library tests: one
that mocks config.SETTINGS_QUERY_ENGINE getter to return
CLP_QUERY_ENGINES.PRESTO and asserts the timeline is not rendered (queryByTestId
returns null), and another that mocks it to a non-PRESTO engine and asserts the
timeline is rendered (getByTestId finds it).
Description
Removes the timeline since not yet supported for presto. Timeline support will be added in phase II of presto webui.
Checklist
breaking change.
Validation performed
Timeline does not render
Summary by CodeRabbit