feat(webui): Add timestamp key selector to guided Presto UI.#1332
Conversation
WalkthroughAdds a timestamp key selection UI and data flow for Guided Presto: state field and updater, new TimestampKey components, SQL to fetch timestamp columns (uses new COLUMN_METADATA table suffix), layout/CSS adjustments, and a TimeRangeInput width limit. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant GC as GuidedControls
participant TK as TimestampKeySelect
participant RQ as React Query
participant SQL as buildTimestampColumnsSql
participant Q as querySql
participant PB as Presto Backend
participant S as Presto Search State
U->>GC: Open/Change dataset
GC->>TK: Render with dataset
TK->>RQ: useQuery(["timestampColumns", dataset])
RQ->>SQL: Build SQL (uses COLUMN_METADATA)
SQL-->>RQ: SQL string
RQ->>Q: Execute SQL
Q->>PB: Send query
PB-->>Q: Rows [{name, type}]
Q-->>RQ: Data
alt Data available
TK->>S: updateTimestampKey(firstName or null)
else Error or empty
TK-->>U: Show error/warning and clear timestampKey
end
U->>TK: User selects timestamp key
TK->>S: updateTimestampKey(selected)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| useEffect(() => { | ||
| if (isSuccess) { | ||
| if ("undefined" !== typeof data[0] && null === timestampKey) { | ||
| updateTimestampKey(data[0]); | ||
| } | ||
| } | ||
| }, [ | ||
| isSuccess, | ||
| data, | ||
| timestampKey, | ||
| updateTimestampKey | ||
| ]); |
There was a problem hiding this comment.
| useEffect(() => { | |
| if (isSuccess) { | |
| if ("undefined" !== typeof data[0] && null === timestampKey) { | |
| updateTimestampKey(data[0]); | |
| } | |
| } | |
| }, [ | |
| isSuccess, | |
| data, | |
| timestampKey, | |
| updateTimestampKey | |
| ]); | |
| useEffect(() => { | |
| if (isSuccess) { | |
| const timestampKey = data.at(0) | |
| if ("undefined" !== typeof timestampKey && null === timestampKey){ | |
| const {updateTimestampKey} = usePrestoSearchState.getState() | |
| updateTimestampKey(timestampKey); | |
| } | |
| } | |
| }, [ | |
| isSuccess, | |
| data, | |
| timestampKey, | |
| ]); |
two nitpicks:
- I remember you said we should use getState in callbacks to avoid dependencies?
- how about using
array.at?
There was a problem hiding this comment.
- for one. we need the states in other places anyways, so the component will rerender regardless.
- what are the benefits of array.at vs. []?
There was a problem hiding this comment.
I remember we have this requirement in C++?
https://www.notion.so/yscope/WIP-Coding-Guidelines-9a308b847a5343958ba3cb97a850be66#17a04e4d9e6b80b08249c9aae5f338d2
Co-authored-by: hoophalab <200652805+hoophalab@users.noreply.github.com>
davemarco
left a comment
There was a problem hiding this comment.
addressed review
| useEffect(() => { | ||
| if (isSuccess) { | ||
| if ("undefined" !== typeof data[0] && null === timestampKey) { | ||
| updateTimestampKey(data[0]); | ||
| } | ||
| } | ||
| }, [ | ||
| isSuccess, | ||
| data, | ||
| timestampKey, | ||
| updateTimestampKey | ||
| ]); |
There was a problem hiding this comment.
- for one. we need the states in other places anyways, so the component will rerender regardless.
- what are the benefits of array.at vs. []?
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/TimestampKey/TimestampKeySelect.tsx (2)
100-100: Normalize null to undefined for Select value.Ant Design's Select expects
undefinedto represent "no selection". Passingnullcan cause warnings or unexpected behaviour.Apply this diff:
- value={timestampKey} + value={timestampKey ?? undefined}Based on past review comments.
41-52: Simplify the guard and reduce hook dependencies.The current guard checks
typeof timestampKeys[0]without first verifying the array has elements. Additionally, usinggetState()in the effect callback reduces unnecessary dependencies.Apply this diff:
useEffect(() => { if (isSuccess) { - if ("undefined" !== typeof timestampKeys[0] && null === timestampKey) { - updateTimestampKey(timestampKeys[0]); + const firstKey = timestampKeys?.at(0); + if ("undefined" !== typeof firstKey && null === timestampKey) { + const {updateTimestampKey} = usePrestoSearchState.getState(); + updateTimestampKey(firstKey); } } }, [ isSuccess, timestampKeys, timestampKey, - updateTimestampKey, ]);This uses
array.at(0)for safer access, ensuresfirstKeyis defined, and retrievesupdateTimestampKeyfrom state to avoid re-running on updater reference changes.Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/TimestampKey/TimestampKeySelect.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/SearchControls/Presto/GuidedControls/TimestampKey/TimestampKeySelect.tsx
🧬 Code graph analysis (1)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/TimestampKey/TimestampKeySelect.tsx (2)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/TimestampKey/sql.ts (1)
fetchTimestampColumns(50-50)components/webui/client/src/pages/SearchPage/SearchState/typings.ts (1)
SEARCH_UI_STATE(31-31)
⏰ 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). (3)
- GitHub Check: package-image
- GitHub Check: lint-check (macos-15)
- GitHub Check: rust-checks (macos-15)
🔇 Additional comments (1)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/TimestampKey/TimestampKeySelect.tsx (1)
23-23: No action required –selectDatasetis the actual state property and is defined and used consistently across the codebase.Likely an incorrect or invalid review comment.
| import useSearchStore from "../../../../SearchState"; | ||
| import usePrestoSearchState from "../../../../SearchState/Presto"; | ||
| import {SEARCH_UI_STATE} from "../../../../SearchState/typings"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider consolidating SearchState imports.
The imports from SearchState and SearchState/Presto could be consolidated if SEARCH_UI_STATE were re-exported from the root SearchState module, reducing the need for consumers to import from multiple locations.
Based on past review comments.
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/TimestampKey/TimestampKeySelect.tsx
around lines 10 to 12, the file imports useSearchStore, usePrestoSearchState,
and SEARCH_UI_STATE from different paths; consolidate by re-exporting
SEARCH_UI_STATE from the root SearchState module and then update this file to
import SEARCH_UI_STATE from "../../../../SearchState" alongside the hooks so all
SearchState symbols come from one module, ensuring the root index.ts (or
equivalent) exports SEARCH_UI_STATE from "./typings".
| const {data: timestampKeys, isPending, isSuccess, isError} = useQuery({ | ||
| queryKey: [ | ||
| "timestampColumns", | ||
| dataset, | ||
| ], | ||
| queryFn: () => (dataset ? | ||
| fetchTimestampColumns(dataset) : | ||
| []), | ||
| enabled: null !== dataset, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add type parameters to useQuery for type safety.
The useQuery call lacks generic type parameters, weakening type inference. Since fetchTimestampColumns returns Promise<string[]>, explicitly type the query.
Apply this diff:
- const {data: timestampKeys, isPending, isSuccess, isError} = useQuery({
+ const {data: timestampKeys, isPending, isSuccess, isError} = useQuery<string[], Error>({
queryKey: [
"timestampColumns",
dataset,
],
- queryFn: () => (dataset ?
- fetchTimestampColumns(dataset) :
- []),
+ queryFn: ({queryKey}) => fetchTimestampColumns(queryKey[1] as string),
enabled: null !== dataset,
});This eliminates the ternary (since enabled guards execution) and uses the queryKey parameter to avoid repeating the dataset reference.
Based on past review comments.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const {data: timestampKeys, isPending, isSuccess, isError} = useQuery({ | |
| queryKey: [ | |
| "timestampColumns", | |
| dataset, | |
| ], | |
| queryFn: () => (dataset ? | |
| fetchTimestampColumns(dataset) : | |
| []), | |
| enabled: null !== dataset, | |
| }); | |
| const {data: timestampKeys, isPending, isSuccess, isError} = useQuery<string[], Error>({ | |
| queryKey: [ | |
| "timestampColumns", | |
| dataset, | |
| ], | |
| queryFn: ({queryKey}) => fetchTimestampColumns(queryKey[1] as string), | |
| enabled: null !== dataset, | |
| }); |
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/Presto/GuidedControls/TimestampKey/TimestampKeySelect.tsx
around lines 30-39, the useQuery call lacks generic type parameters and uses a
ternary to guard dataset; update it to use useQuery<string[], Error> and
simplify the queryFn to use the queryKey param instead of repeating dataset
(e.g. queryFn: ({ queryKey }) => fetchTimestampColumns(queryKey[1] as string)),
remove the ternary as enabled: dataset != null already guards execution, and
keep enabled: dataset != null to prevent running when dataset is null.
|
|
||
| useEffect(() => { | ||
| if (isSuccess) { | ||
| if ("undefined" !== typeof timestampKeys[0] && null === timestampKey) { |
There was a problem hiding this comment.
I remember we have this requirement in C++?
https://www.notion.so/yscope/WIP-Coding-Guidelines-9a308b847a5343958ba3cb97a850be66#17a04e4d9e6b80b08249c9aae5f338d2
There was a problem hiding this comment.
i tried it but, but typescript support it since it dosent remember thats its not undefined. Like in the next line there is a type error since it thinks it can be undefined, even though we just checked
There was a problem hiding this comment.
we can cache it in a local var and return if it's undefined?
There was a problem hiding this comment.
i think will make less readable
Description
Adds timekey selector to UI, and range input. Timekey uses sql query to pull timestamps. It also filters based on whether column is a timestamp or not. I asked devin and user set timestamp is always the same constant in the database.
Checklist
breaking change.
Validation performed
Tested a couple datasets and timestamps. It appears to reset when you change dataset.
Summary by CodeRabbit
New Features
UI