-
Notifications
You must be signed in to change notification settings - Fork 83
fix(new-webui): Add support for querying metadata from multiple datasets (fixes #1024). #1042
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
Changes from 21 commits
a4d1cd8
dab039b
89a8b04
0786e2c
9f33330
6e90915
038656f
1f0dc43
01a134f
e1bdcd5
0191277
7a9846d
f9512b9
e463470
4bdc128
36d91fb
6b72a6d
4235ed6
e7824cd
55a3e80
279e7ad
77b15c9
21e8eb2
481e230
661262c
883f849
f014a50
f62a620
968cc80
84d80be
a80a7dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| import {QueryClient} from "@tanstack/react-query"; | ||
|
|
||
|
|
||
| const DEFAULT_STALE_TIME_MILLIS = 10_000; | ||
|
|
||
| const queryClient = new QueryClient({ | ||
| defaultOptions: { | ||
| queries: { | ||
| staleTime: DEFAULT_STALE_TIME_MILLIS, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| export default queryClient; |
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,104 +1,74 @@ | ||||||||||||||
| import { | ||||||||||||||
| useCallback, | ||||||||||||||
| useEffect, | ||||||||||||||
| useState, | ||||||||||||||
| } from "react"; | ||||||||||||||
|
|
||||||||||||||
| import dayjs, {Dayjs} from "dayjs"; | ||||||||||||||
| import {Nullable} from "src/typings/common"; | ||||||||||||||
| import {useQuery} from "@tanstack/react-query"; | ||||||||||||||
| import dayjs from "dayjs"; | ||||||||||||||
|
|
||||||||||||||
| import {querySql} from "../../../api/sql"; | ||||||||||||||
| import { | ||||||||||||||
| CLP_STORAGE_ENGINES, | ||||||||||||||
| SETTINGS_STORAGE_ENGINE, | ||||||||||||||
| } from "../../../config"; | ||||||||||||||
| import useIngestStatsStore from "../ingestStatsStore"; | ||||||||||||||
| import {fetchDatasetNames} from "../../SearchPage/SearchControls/Dataset/sql"; | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainFix the unresolved import path. The ESLint error indicates that the import path 🏁 Script executed: #!/bin/bash
# Verify the import path exists and check the correct relative path
fd -t f "sql.ts" | grep -E "(SearchPage|Dataset)" | head -10Length of output: 146 #!/bin/bash
# Locate the IngestPage Details index file to confirm its path
fd -t f index.tsx | grep -E "IngestPage/Details"🏁 Script executed: #!/bin/bash
# Find the path of the IngestPage Details index file and verify the import target
file=$(fd "Details/index.tsx" | head -n1)
echo "Found index.tsx at: $file"
dir=$(dirname "$file")
import_rel="../../SearchPage/SearchControls/Dataset/sql.ts"
echo "Checking existence of import target: $dir/$import_rel"
if [ -f "$dir/$import_rel" ]; then
echo "✅ Import target exists at $dir/$import_rel"
else
echo "❌ Import target missing at $dir/$import_rel"
fi
# List all sql.ts occurrences for context
echo "All sql.ts files in project:"
fd -t f "sql.ts"Length of output: 1036 🏁 Script executed: #!/bin/bash
# 1. Locate the IngestPage Details index file
echo "Searching for Details/index.tsx…"
fd -t f index.tsx | grep -E "IngestPage/Details"
# 2. Find where fetchDatasetNames is defined in the repo
echo "Looking for fetchDatasetNames definitions…"
rg "fetchDatasetNames" -n .Length of output: 1088 #!/bin/bash
# Display the top of Details/sql.ts
echo "=== components/webui/client/src/pages/IngestPage/Details/sql.ts ==="
sed -n '1,20p' "$(fd -t f sql.ts | grep 'webui/client/src/pages/IngestPage/Details/sql.ts')"
# Display the top of api/sql.ts to see if fetchDatasetNames is exported
echo -e "\n=== components/webui/client/src/api/sql.ts ==="
sed -n '1,20p' "$(fd -t f sql.ts | grep 'webui/client/src/api/sql.ts')"#!/bin/bash
# Locate and display the Details/sql.ts file
details_sql=$(fd -t f sql.ts | grep 'webui/client/src/pages/IngestPage/Details/sql.ts')
echo "=== $details_sql ==="
sed -n '1,50p' "$details_sql"
# Locate and inspect client api/sql.ts for fetchDatasetNames export
api_sql=$(fd -t f sql.ts | grep 'webui/client/src/api/sql.ts')
echo -e "\n=== $api_sql ==="
grep -R "export.*fetchDatasetNames" -n "$api_sql" || echo "No fetchDatasetNames export found in api/sql.ts"
# Search all of webui/client for fetchDatasetNames occurrences
echo -e "\n=== All fetchDatasetNames in webui/client ==="
grep -R "fetchDatasetNames" -n components/webui/client/srcFix broken import path for fetchDatasetNames The import import {fetchDatasetNames} from "../../SearchPage/SearchControls/Dataset/sql";does not resolve because there is no • File: Suggested diff: - import {fetchDatasetNames} from "../../SearchPage/SearchControls/Dataset/sql";
+ import {fetchDatasetNames} from "../../../api/sql";Adjust the path according to where 🧰 Tools🪛 GitHub Actions: clp-lint[error] 8-8: ESLint: Unable to resolve path to module '../../SearchPage/SearchControls/Dataset/sql' (import/no-unresolved) 🤖 Prompt for AI Agents |
||||||||||||||
| import Files from "./Files"; | ||||||||||||||
| import styles from "./index.module.css"; | ||||||||||||||
| import Messages from "./Messages"; | ||||||||||||||
| import { | ||||||||||||||
| DetailsResp, | ||||||||||||||
| getDetailsSql, | ||||||||||||||
| DETAILS_DEFAULT, | ||||||||||||||
| fetchClpDetails, | ||||||||||||||
| fetchClpsDetails, | ||||||||||||||
| } from "./sql"; | ||||||||||||||
| import TimeRange from "./TimeRange"; | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
| * Default state for details. | ||||||||||||||
| */ | ||||||||||||||
| const DETAILS_DEFAULT = Object.freeze({ | ||||||||||||||
| beginDate: dayjs(null), | ||||||||||||||
| endDate: dayjs(null), | ||||||||||||||
|
|
||||||||||||||
| numFiles: 0, | ||||||||||||||
| numMessages: 0, | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
| * Renders grid with compression details. | ||||||||||||||
| * | ||||||||||||||
| * @return | ||||||||||||||
| */ | ||||||||||||||
| const Details = () => { | ||||||||||||||
| const {refreshInterval} = useIngestStatsStore(); | ||||||||||||||
| const [beginDate, setBeginDate] = useState<Dayjs>(DETAILS_DEFAULT.beginDate); | ||||||||||||||
| const [endDate, setEndDate] = useState<Dayjs>(DETAILS_DEFAULT.endDate); | ||||||||||||||
| const [numFiles, setNumFiles] = useState<Nullable<number>>(DETAILS_DEFAULT.numFiles); | ||||||||||||||
| const [numMessages, setNumMessages] = useState<Nullable<number>>(DETAILS_DEFAULT.numMessages); | ||||||||||||||
| const {data: datasetNames = [], isSuccess: isSuccessDatasetNames} = useQuery({ | ||||||||||||||
| queryKey: ["datasets"], | ||||||||||||||
| queryFn: fetchDatasetNames, | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainAddress the unsafe assignment warning. The TypeScript ESLint error suggests there's an unsafe assignment of an error-typed value. Ensure that the 🏁 Script executed: #!/bin/bash
# Check the return type of fetchDatasetNames function
ast-grep --pattern 'export $_ fetchDatasetNames'Length of output: 50 🏁 Script executed: #!/bin/bash
# Locate all references and the definition of fetchDatasetNames
rg 'fetchDatasetNames' -nLength of output: 792 🏁 Script executed: #!/bin/bash
# Display lines around the usage of fetchDatasetNames in the Details page
sed -n '1,200p' components/webui/client/src/pages/IngestPage/Details/index.tsxLength of output: 2314 Specify useQuery generics to avoid unsafe The • In - const { data: datasetNames = [], isSuccess: isSuccessDatasetNames } = useQuery({
+ const { data: datasetNames = [], isSuccess: isSuccessDatasetNames } = useQuery<string[], Error>({
queryKey: ["datasets"],
queryFn: fetchDatasetNames,
enabled: CLP_STORAGE_ENGINES.CLP_S === SETTINGS_STORAGE_ENGINE,
});Optionally, apply the same change in This ensures 📝 Committable suggestion
Suggested change
🧰 Tools🪛 GitHub Actions: clp-lint[error] 28-28: ESLint: Unsafe assignment of an error typed value (@typescript-eslint/no-unsafe-assignment) 🤖 Prompt for AI Agents |
||||||||||||||
| enabled: CLP_STORAGE_ENGINES.CLP_S === SETTINGS_STORAGE_ENGINE, | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
| * Fetches details stats from the server. | ||||||||||||||
| * | ||||||||||||||
| * @throws {Error} If the response is undefined. | ||||||||||||||
| */ | ||||||||||||||
| const fetchDetailsStats = useCallback(() => { | ||||||||||||||
| querySql<DetailsResp>(getDetailsSql()).then((resp) => { | ||||||||||||||
| const [details] = resp.data; | ||||||||||||||
| if ("undefined" === typeof details) { | ||||||||||||||
| throw new Error("Details response is undefined"); | ||||||||||||||
| const {data: details = DETAILS_DEFAULT, isPending} = useQuery({ | ||||||||||||||
| queryKey: [ | ||||||||||||||
| "details", | ||||||||||||||
| datasetNames, | ||||||||||||||
| ], | ||||||||||||||
| queryFn: async () => { | ||||||||||||||
| if (CLP_STORAGE_ENGINES.CLP === SETTINGS_STORAGE_ENGINE) { | ||||||||||||||
| return fetchClpDetails(); | ||||||||||||||
| } | ||||||||||||||
| setBeginDate(dayjs.utc(details.begin_timestamp)); | ||||||||||||||
| setEndDate(dayjs.utc(details.end_timestamp)); | ||||||||||||||
| setNumFiles(details.num_files); | ||||||||||||||
| setNumMessages(details.num_messages); | ||||||||||||||
| }) | ||||||||||||||
| .catch((e: unknown) => { | ||||||||||||||
| console.error("Failed to fetch details stats", e); | ||||||||||||||
| }); | ||||||||||||||
| }, []); | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| useEffect(() => { | ||||||||||||||
| fetchDetailsStats(); | ||||||||||||||
| const intervalId = setInterval(fetchDetailsStats, refreshInterval); | ||||||||||||||
|
|
||||||||||||||
| return () => { | ||||||||||||||
| clearInterval(intervalId); | ||||||||||||||
| }; | ||||||||||||||
| }, [ | ||||||||||||||
| refreshInterval, | ||||||||||||||
| fetchDetailsStats, | ||||||||||||||
| ]); | ||||||||||||||
| return fetchClpsDetails(datasetNames); | ||||||||||||||
| }, | ||||||||||||||
| enabled: CLP_STORAGE_ENGINES.CLP === SETTINGS_STORAGE_ENGINE || isSuccessDatasetNames, | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| if (CLP_STORAGE_ENGINES.CLP === SETTINGS_STORAGE_ENGINE) { | ||||||||||||||
| return ( | ||||||||||||||
| <div className={styles["detailsGrid"]}> | ||||||||||||||
| <div className={styles["timeRange"]}> | ||||||||||||||
| <TimeRange | ||||||||||||||
| beginDate={beginDate} | ||||||||||||||
| endDate={endDate}/> | ||||||||||||||
| beginDate={dayjs.utc(details.begin_timestamp)} | ||||||||||||||
| endDate={dayjs.utc(details.end_timestamp)} | ||||||||||||||
| isLoading={isPending}/> | ||||||||||||||
| </div> | ||||||||||||||
| <Messages numMessages={numMessages}/> | ||||||||||||||
| <Files numFiles={numFiles}/> | ||||||||||||||
| <Messages | ||||||||||||||
| isLoading={isPending} | ||||||||||||||
| numMessages={details.num_messages}/> | ||||||||||||||
| <Files | ||||||||||||||
| isLoading={isPending} | ||||||||||||||
| numFiles={details.num_files}/> | ||||||||||||||
| </div> | ||||||||||||||
| ); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| return ( | ||||||||||||||
| <div> | ||||||||||||||
| <TimeRange | ||||||||||||||
| beginDate={beginDate} | ||||||||||||||
| endDate={endDate}/> | ||||||||||||||
| beginDate={dayjs.utc(details.begin_timestamp)} | ||||||||||||||
| endDate={dayjs.utc(details.end_timestamp)} | ||||||||||||||
| isLoading={isPending}/> | ||||||||||||||
| </div> | ||||||||||||||
| ); | ||||||||||||||
|
Comment on lines
67
to
73
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Quick question: I might be missing some context here. Why don’t we show Messages and Files for CLP-s? @davemarco
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The clp-s storage engine doesn't currently keep track of the number of log events and files in the archives. |
||||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
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.
This inheritance-style dependencies is too crazy.
We have
DetailsCard -> StatCard -> DashboardCardYou are passing
isLoadingall the way down toDashboardCardwhich is a typical problem of inheritanceSuggestion:
DetailsCardas all cards requires title and stat/contentDetailsCardacceptstitleas a propertyDetailsCardaccepts content text (i.e.No timestamp dataand0.00%) as react node in children, so that space saving can have a different fontsize.DetailsCarddirectly andisLoadingbecomes a page state in zustand store...propsparameter in all concrete cards such asFileMessage... which are directed toDetailsCardThere 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.
I did also think these cards can be refactored when working on the code. But probably a different PR.
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.
Can we create a quick refactor PR to address this before merging? It shouldn’t take long — I’m happy to submit one and you review that. However, we shouldn't let the current
isLoadingsolution make it into the main branch.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.
okay this is reasonable, but you will have to merge into this PR