feat(new-webui): Integrate real-time query results into query UI.#931
Conversation
junhaoliao
left a comment
There was a problem hiding this comment.
it seems we're missing the initial data load
|
|
||
| useEffect(() => { | ||
| // eslint-disable-next-line no-void | ||
| void fetchDetailsStats(); |
There was a problem hiding this comment.
was removal of the initial data load intentional?
There was a problem hiding this comment.
At first yes, but i didnt realize the refresh interval was 10 seconds. I will add it back
|
|
||
|
|
||
| useEffect(() => { | ||
| // eslint-disable-next-line no-void |
There was a problem hiding this comment.
ditto - was removal of the initial data load intentional?
| }, []); | ||
|
|
||
| useEffect(() => { | ||
| // eslint-disable-next-line no-void |
There was a problem hiding this comment.
ditto - was removal of the initial data load intentional?
There was a problem hiding this comment.
it appears that num_messages can be NULL in the SQL results. shall we
update getDetailsSql as
const getDetailsSql = () => `
SELECT
a.begin_timestamp AS begin_timestamp,
a.end_timestamp AS end_timestamp,
b.num_files AS num_files,
b.num_messages AS num_messages
FROM
(
SELECT
MIN(${CLP_ARCHIVES_TABLE_COLUMN_NAMES.BEGIN_TIMESTAMP}) AS begin_timestamp,
MAX(${CLP_ARCHIVES_TABLE_COLUMN_NAMES.END_TIMESTAMP}) AS end_timestamp
FROM ${SQL_CONFIG.SqlDbClpArchivesTableName}
) a,
(
SELECT
COUNT(DISTINCT ${CLP_FILES_TABLE_COLUMN_NAMES.ORIG_FILE_ID}) AS num_files,
CAST(
COALESCE(
SUM(${CLP_FILES_TABLE_COLUMN_NAMES.NUM_MESSAGES}),
0
) AS INTEGER
) AS num_messages
FROM ${SQL_CONFIG.SqlDbClpFilesTableName}
) b;
`;
also, the DetailsItem typing doesn't seem correct. i think it should be:
interface DetailsItem {
begin_timestamp: Nullable<number>;
end_timestamp: Nullable<number>;
num_files: number;
num_messages: number;
}
to ensure consistent date formatting when the dates are not available, i believe we should set beginDate & endDate as always non-null
const DETAILS_DEFAULT = Object.freeze({
beginDate: dayjs(null),
endDate: dayjs(null),
...
});
...
const [beginDate, setBeginDate] = useState<Dayjs>(DETAILS_DEFAULT.beginDate);
const [endDate, setEndDate] = useState<Dayjs>(DETAILS_DEFAULT.endDate);
then in the TimeRange component:
interface TimeRangeProps {
beginDate: Dayjs;
endDate: Dayjs;
}
...
const TimeRange = ({beginDate, endDate}: TimeRangeProps) => {
const formattedBeginDate = beginDate.isValid() ?
beginDate.format(DATE_FORMAT) :
"Unknown Begin Date";
const formattedEndDate = endDate.isValid() ?
endDate.format(DATE_FORMAT) :
"Unknown End Date";
return (
<DetailsCard
stat={`${formattedBeginDate} - ${formattedEndDate}`}
title={"Time Range"}/>
);
};
There was a problem hiding this comment.
I agree with the changes, except that can we make DetailsItem's num_files and num_messages as nullable and fill default in the frontend?
Also, marco or I need to make the change?
There was a problem hiding this comment.
I agree with the changes, except that can we make DetailsItem's num_files and num_messages as nullable and fill default in the frontend?
sure
Also, marco or I need to make the change?
@hoophalab since you're the original author of the code, please make changes from your end and let @davemarco review
There was a problem hiding this comment.
okay i made changes with small change to TimeRange
| className={styles["queryBoxProgress"] || ""} | ||
| percent={progress ?? 0} | ||
| className={styles["progressBar"] || ""} | ||
| percent={progress} |
There was a problem hiding this comment.
since you are already changing the querybox's code, can you also add the case sensitive button? I don't need to wait for the merge of this one and put up another pr xD
| const fetchJobsStats = useCallback(() => { | ||
| const beginTimestamp = dayjs().subtract(DAYS_TO_SHOW, "days") | ||
| .unix(); | ||
| const {data: resp} = await querySql<QueryJobsResp>(getQueryJobsSql(beginTimestamp)); | ||
| const newJobs = resp | ||
| .map((item): JobData => convertQueryJobsItemToJobData(item)); | ||
|
|
||
| setJobs(newJobs); | ||
| querySql<QueryJobsResp>(getQueryJobsSql(beginTimestamp)) | ||
| .then((resp) => { | ||
| const newJobs = resp.data.map( | ||
| (item): JobData => convertQueryJobsItemToJobData(item) | ||
| ); | ||
|
|
||
| setJobs(newJobs); | ||
| }) | ||
| .catch((e: unknown) => { | ||
| console.error("Failed to fetch jobs stats", e); | ||
| }); |
There was a problem hiding this comment.
To be honest, I don't feel this is simpler, it needs more lines and more nested level. but if you are going to maintain the code in the future, feel free to change it to whatever looks good to you.
There was a problem hiding this comment.
I was seeing this error on the linter -
There was a problem hiding this comment.
I dont think u can pass async into setInterval. supposedly there is an async version, but i did this instead
There was a problem hiding this comment.
anyways something with these intervals was breaking the build i believe, i dont remember exactly what but thats i changed these files
There was a problem hiding this comment.
this is the first version of this file which should solve the issue.
Also, @junhaoliao said offline that we want a interval dropdown that let the user select the interval. But I forgot whether it's a per widget one or a global one. Either way, some other functions need intervalId in order to cancel the previous interval.
There was a problem hiding this comment.
Also, @junhaoliao said offline that we want a interval dropdown that let the user select the interval
the refresh interval should be consistent for all widgets. we can add the drop-down in another PR
There was a problem hiding this comment.
the old code with the void wrapper should work, but also looks strange lol. In the react docs, they seem to use promises for api fetch https://react.dev/learn/you-might-not-need-an-effect#fetching-data. I will let @junhaoliao decide void wrapper vs. promises
There was a problem hiding this comment.
If typing is an issue, I'm fine with the .then syntax if there is only one Promise in the function.
| <Input {...inputProps}/> | ||
| <div | ||
| className={styles["progressBarMask"]} | ||
| style={{borderRadius: token.borderRadius}} |
There was a problem hiding this comment.
This one is 6px on my machine and Input's is 8px.
Need to take a look at what Input actually uses.
There was a problem hiding this comment.
oh there is a borderRadiusLG, maybe should use this isntead
There was a problem hiding this comment.
but didnt test if actual works or exists
There was a problem hiding this comment.
but nice catch, i didnt realize
junhaoliao
left a comment
There was a problem hiding this comment.
as discussed offline, it's mostly clean besides the issues that's been brought up
i posted a few more stylistic comments, and also a few more quick questions for the changes
There was a problem hiding this comment.
as discussed offline: this is fine to be checked-in in the current PR, but I will add a mechanism to replace it as some static file in the build and load the settings from the static file at init time
| /** | ||
| * The progress of the progress bar from `0` to `100`. Hides the bar if `0`. | ||
| */ | ||
| progress: number; |
There was a problem hiding this comment.
if we're not using a pseudo progress number, how do we indicate a query has started but the server reported progress is 0? Or do you mean that when we actually implement query progress in the server, we should never report 0 as a valid progress number?
There was a problem hiding this comment.
hmm i wasnt thinking too much, i think the old code just set the progress to 0, so this change seemed simple. We can also set to null in the progress code. I think the only difference will be that the bar will just dissapear and not slide down to 0, but maybe that is better
There was a problem hiding this comment.
I can change to null, may be better
There was a problem hiding this comment.
yup i think the differentiation between null and 0 will be helpful
| style={{display: null === progress ? | ||
| "none" : | ||
| "block"}}/> |
There was a problem hiding this comment.
if we don't always mount the progress bar, would it cause the result table to move by a few pixels when the progress becomes non-zero?
There was a problem hiding this comment.
I didnt notice anything
There was a problem hiding this comment.
Like the results table might move maybe a few pixels once mounted. Let me double check on my end first
There was a problem hiding this comment.
i just verified that is not an issue in the latest implementation
| onClick={() => { | ||
| const timelineConfig = computeTimelineConfig( | ||
| timeRange[0].valueOf(), | ||
| timeRange[1].valueOf() | ||
| ); | ||
|
|
||
| handleQuerySubmit({ | ||
| ignoreCase: false, | ||
| queryString: queryString, | ||
| timeRangeBucketSizeMillis: timelineConfig.bucketDuration.asMilliseconds(), | ||
| timestampBegin: timeRange[0].valueOf(), | ||
| timestampEnd: timeRange[1].valueOf(), | ||
| }); | ||
| }} | ||
| > |
There was a problem hiding this comment.
let's extract a handleSubmitButton click function wrapped by useCallback
| icon={<CloseOutlined/>} | ||
| size={"large"} | ||
| type={"primary"} | ||
| onClick={() => { |
There was a problem hiding this comment.
ditto - let's extract the handler and wrap it by useCallback
| virtual={true}/> | ||
| rowKey={(record) => record._id.toString()} | ||
| scroll={{y: 400}} | ||
|
|
There was a problem hiding this comment.
is the empty line intentional?
| rowKey={(record) => record.id.toString()} | ||
| virtual={true}/> | ||
| rowKey={(record) => record._id.toString()} | ||
| scroll={{y: 400}} |
There was a problem hiding this comment.
can we extract a named constant from this magic number?
There was a problem hiding this comment.
I made this dynamic to screen size per what kirk mentions earlier. Might as well do it now.
There was a problem hiding this comment.
any reason we want the .env values to override .env.local values? (let me know if i understood the change wrong
There was a problem hiding this comment.
I reverted, mistake on my part
There was a problem hiding this comment.
https://github.com/motdotla/dotenv#options
unless the options.override flag is set, in which case the last value set will win
actually to let the values in .env.local correctly override the ones set in .env, i believe we should be setting the override: true property as well. i.e.,
dotenv: {
path: [
".env",
".env.local",
],
override: true,
},
@coderabbitai create an issue to track this
There was a problem hiding this comment.
🧩 Analysis chain
@junhaoliao, you're absolutely right about the dotenv configuration. Without the override: true flag, variables in .env.local won't properly override those in .env. I'll create an issue to track this improvement.
🏁 Script executed:
#!/bin/bash
gh issue create \
--title "Fix dotenv configuration to allow .env.local to override .env values" \
--body "## Problem
The current dotenv configuration in \`components/log-viewer-webui/server/src/fastify-v2/plugins/external/env.ts\` does not include the \`override: true\` flag. According to the [dotenv documentation](https://github.com/motdotla/dotenv#options), without this flag, when the same environment variable is defined in both \`.env\` and \`.env.local\`, the value from \`.env.local\` will not override the one from \`.env\`.
## Current Configuration
\`\`\`typescript
dotenv: {
path: [
\".env\",
\".env.local\",
],
},
\`\`\`
## Proposed Solution
\`\`\`typescript
dotenv: {
path: [
\".env\",
\".env.local\",
],
override: true,
},
\`\`\`
## References
- PR: https://github.com/y-scope/clp/pull/931
- Comment: https://github.com/y-scope/clp/pull/931#discussion_r2112877916
- Requested by: @junhaoliao" \
--assignee junhaoliao
Length of output: 994
Perfect! I've successfully created issue #1010 to track the dotenv configuration fix: #1010
The issue documents the problem with the current configuration and includes your suggested solution to add override: true to ensure that .env.local values properly override those in .env. It's been assigned to you as requested.
🧬 Code Graph Analysis Results
Relevant snippet from components/log-viewer-webui/server/src/fastify-v2/plugins/external/env.ts (lines 1-56)
/* eslint-disable sort-keys */
import env from "@fastify/env";
declare module "fastify" {
export interface FastifyInstance {
config: {
PORT: number;
HOST: string;
CLP_DB_USER: string;
CLP_DB_PASS: string;
RATE_LIMIT_MAX: number;
};
}
}
const schema = {
type: "object",
required: [
"PORT",
"HOST",
"CLP_DB_USER",
"CLP_DB_PASS",
],
properties: {
PORT: {
type: "number",
default: 3000,
minimum: 1,
maximum: 65535,
},
HOST: {
type: "string",
minLength: 1,
default: "localhost",
},
// Databases
CLP_DB_USER: {
type: "string",
minLength: 1,
default: "clp-user",
},
CLP_DB_PASS: {
type: "string",
},
// Security
RATE_LIMIT_MAX: {
type: "number",
default: 100,
},
},
};
export const autoConfig = {
confKey: "config",
schema: schema,
// Needed to read .env in root folder
dotenv: {
path: [
".env",
".env.local",
],
},
// Source for the configuration data
// Optional, default: process.env
data: process.env,
};
export default env;Possibly relevant snippet from components/webui/server/main.js (lines 30-33)
const {
CLP_DB_USER,
CLP_DB_PASS,
} = process.env;Possibly relevant snippet from components/log-viewer-webui/server/src/fastify-v2/plugins/external/rateLimit.ts (lines 7-12)
export const autoConfig = (fastify: FastifyInstance) => {
return {
max: fastify.config.RATE_LIMIT_MAX,
timeWindow: "1 minute",
};
};Possibly relevant snippet from components/log-viewer-webui/server/src/fastify-v2/plugins/external/mysql.ts (lines 18-27)
export const autoConfig = (fastify: FastifyInstance) => {
return {
database: settings.SqlDbName,
host: settings.SqlDbHost,
password: fastify.config.CLP_DB_PASS,
port: Number(settings.SqlDbPort),
promise: true,
user: fastify.config.CLP_DB_USER,
};
};Possibly relevant snippet from components/log-viewer-webui/server/src/fastify-v2/plugins/external/mongo.ts (lines 6-11)
export const autoConfig = () => {
return {
forceClose: true,
url: `mongodb://${settings.MongoDbHost}:${settings.MongoDbPort}/${settings.MongoDbName}`,
};
};Possibly relevant snippet from components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts (lines 158-160)
export interface FastifyInstance {
QueryJobsDbManager: QueryJobsDbManager;
}These snippets provide context on environment variable usage, configuration loading, and how the environment variables are accessed and used in the application. The main snippet from env.ts shows the current dotenv configuration without the override: true option, which is the focus of the user comment.
|
|
||
|
|
||
| // Counter for generating unique query IDs. | ||
| let globalQueryIdCounter = 0; |
There was a problem hiding this comment.
gg... sorry for missing this in the previous review.
can we make this variable a member of the MongoSocketIoServer singleton class?
There was a problem hiding this comment.
I think that should be fine
There was a problem hiding this comment.
mind explaining the bug? i thought usage of the replacer argument was intended, to make the serialization deterministic in terms of key ordering in the serialized string.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (7)
components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/SearchButton/SubmitButton.tsx (3)
29-32: Add bounds checking for timeRange array access.Direct array access without bounds checking could cause runtime errors if the timeRange array is empty or has fewer than 2 elements.
+ if (timeRange.length < 2) { + console.error("Invalid timeRange: expected array with at least 2 elements"); + return; + } const timelineConfig = computeTimelineConfig( timeRange[0].valueOf(), timeRange[1].valueOf() );
35-35: Consider making ignoreCase configurable.The
ignoreCaseoption is hardcoded tofalse. Consider making this configurable through the search state or component props for better flexibility.
51-51: Remove unnecessary fallback for CSS class.The
|| ""fallback is unnecessary since React will ignore undefined className values automatically.- className={styles["gradientButton"] || ""} + className={styles["gradientButton"]}components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts (1)
59-60: Excellent improvement for query ID generation!Adding the
#queryIdCounteras a private class field is a much better approach than the previous implementation that had to scan through existing query IDs to find the maximum. This addresses the previous review comment about making it a class member.components/log-viewer-webui/client/src/pages/IngestPage/Details/index.tsx (1)
46-49: Update JSDoc to reflect actual error handling.The JSDoc comment still mentions
@throws {Error}but the implementation now catches errors internally and logs them instead of throwing to the caller./** * Fetches details stats from the server. - * - * @throws {Error} If the response is undefined. */components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/QueryInput/index.tsx (2)
52-54: Complete the cleanup by resetting the interval reference.The cleanup effect clears the interval but doesn't reset the ref value, which could cause issues if the component remounts or during rapid state changes.
Apply this fix:
useEffect(() => { + return () => { clearInterval(intervalIdRef.current); + intervalIdRef.current = 0; + }; }, []);
28-49: 🧹 Nitpick (assertive)Consider adding additional safeguards for interval management.
While the logic handles most cases correctly, there's a potential edge case where rapid state changes could lead to interval conflicts. The warning on line 31 helps identify this, but the component could be more robust.
Consider this enhanced approach:
useEffect(() => { + // Clear any existing interval first + if (intervalIdRef.current !== 0) { + clearInterval(intervalIdRef.current); + intervalIdRef.current = 0; + } + if (searchUiState === SEARCH_UI_STATE.QUERY_ID_PENDING) { - if (0 !== intervalIdRef.current) { - console.warn("Interval already set for submitted query"); - return; - } intervalIdRef.current = window.setInterval(() => { setPseudoProgress((v) => { if (100 <= (v ?? 0) + PROGRESS_INCREMENT) { return 100; } return (v ?? 0) + PROGRESS_INCREMENT; }); }, PROGRESS_INTERVAL_MILLIS); } else if (searchUiState === SEARCH_UI_STATE.DONE) { - clearInterval(intervalIdRef.current); - intervalIdRef.current = 0; setPseudoProgress(null); } }, [searchUiState]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
components/log-viewer-webui/client/src/components/QueryBox/index.tsx(2 hunks)components/log-viewer-webui/client/src/pages/IngestPage/Details/Files.tsx(2 hunks)components/log-viewer-webui/client/src/pages/IngestPage/Details/Messages.tsx(2 hunks)components/log-viewer-webui/client/src/pages/IngestPage/Details/TimeRange.tsx(2 hunks)components/log-viewer-webui/client/src/pages/IngestPage/Details/index.tsx(2 hunks)components/log-viewer-webui/client/src/pages/IngestPage/Details/sql.ts(2 hunks)components/log-viewer-webui/client/src/pages/IngestPage/Jobs/index.tsx(1 hunks)components/log-viewer-webui/client/src/pages/IngestPage/SpaceSavings/index.tsx(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/QueryInput/index.tsx(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/SearchButton/CancelButton.tsx(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/SearchButton/SubmitButton.tsx(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/index.tsx(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/typings.tsx(3 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/useSearchResults.ts(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchState/useResultsMetadata.ts(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts(1 hunks)components/log-viewer-webui/client/vite.config.ts(2 hunks)components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/log-viewer-webui/client/vite.config.tscomponents/log-viewer-webui/client/src/pages/IngestPage/Details/TimeRange.tsxcomponents/log-viewer-webui/client/src/pages/IngestPage/Details/sql.tscomponents/log-viewer-webui/client/src/pages/IngestPage/Details/index.tsxcomponents/log-viewer-webui/client/src/pages/SearchPage/SearchState/useResultsMetadata.tscomponents/log-viewer-webui/client/src/pages/SearchPage/SearchControls/SearchButton/CancelButton.tsxcomponents/log-viewer-webui/client/src/pages/IngestPage/Details/Messages.tsxcomponents/log-viewer-webui/client/src/pages/SearchPage/SearchControls/SearchButton/SubmitButton.tsxcomponents/log-viewer-webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.tscomponents/log-viewer-webui/client/src/pages/IngestPage/SpaceSavings/index.tsxcomponents/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.tscomponents/log-viewer-webui/client/src/pages/IngestPage/Details/Files.tsxcomponents/log-viewer-webui/client/src/pages/IngestPage/Jobs/index.tsxcomponents/log-viewer-webui/client/src/pages/SearchPage/SearchControls/QueryInput/index.tsxcomponents/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/index.tsxcomponents/log-viewer-webui/client/src/pages/SearchPage/SearchResults/useSearchResults.tscomponents/log-viewer-webui/client/src/components/QueryBox/index.tsxcomponents/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/typings.tsx
🧬 Code Graph Analysis (6)
components/log-viewer-webui/client/src/pages/IngestPage/Details/sql.ts (1)
components/log-viewer-webui/client/src/pages/IngestPage/sqlConfig.ts (2)
CLP_FILES_TABLE_COLUMN_NAMES(62-62)SQL_CONFIG(65-65)
components/log-viewer-webui/client/src/pages/SearchPage/SearchState/useResultsMetadata.ts (2)
components/log-viewer-webui/client/src/api/socket/useCursor.tsx (1)
useCursor(79-79)components/webui/imports/ui/SearchView/SearchView.jsx (1)
resultsMetadata(77-98)
components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/SearchButton/CancelButton.tsx (1)
components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/search-requests.ts (1)
handleQueryCancel(104-104)
components/log-viewer-webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts (4)
components/webui/imports/ui/SearchView/SearchView.jsx (1)
resultsMetadata(77-98)components/log-viewer-webui/client/src/pages/SearchPage/SearchState/useResultsMetadata.ts (1)
useResultsMetadata(51-51)components/log-viewer-webui/common/index.ts (1)
SEARCH_SIGNAL(105-105)components/log-viewer-webui/client/src/pages/SearchPage/SearchState/typings.ts (1)
SEARCH_UI_STATE(26-26)
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/useSearchResults.ts (2)
components/log-viewer-webui/client/src/api/socket/useCursor.tsx (1)
useCursor(79-79)components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/typings.tsx (1)
SearchResult(52-52)
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/typings.tsx (1)
components/log-viewer-webui/client/src/typings/datetime.ts (1)
DATETIME_FORMAT_TEMPLATE(3-3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (33)
components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/SearchButton/CancelButton.tsx (2)
21-38: Well implemented with proper useCallback usage.The handler implementation properly addresses the previous review feedback by extracting the logic into a separate function and wrapping it with
useCallback. The dependency array correctly includes bothsearchJobIdandaggregationJobId, and the null checks provide good defensive programming with clear error messages.
40-50: Appropriate button styling and properties.The Button component uses sensible props for a cancel action -
danger={true}for visual distinction,CloseOutlinedicon for clear intent, and consistent sizing with other controls. The click handler is properly bound.components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/SearchButton/SubmitButton.tsx (1)
28-42: LGTM! Callback implementation addresses previous feedback.The handleSubmitButtonClick function is properly wrapped with useCallback and includes the correct dependencies (queryString, timeRange). This addresses the previous review comment about extracting the click handler.
components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts (3)
182-186: LGTM! Query ID counter implementation is efficient and thread-safe.The refactored logic using the monotonic counter is much more efficient than the previous approach. The comment about JavaScript being single-threaded and the increment operation being atomic is accurate and helpful for understanding the thread safety.
204-204: Log message improvement approved.The updated log message "Initialize Mongo watcher collection" is more descriptive than the previous version and better reflects the action being performed.
14-21: 💡 Verification agent🧩 Analysis chain
Verify the relative import path is correct.
The import path has changed from a package-level import
"@common/index.js"to a relative path"../../../../common/index.js". Please ensure this relative path correctly resolves to the common module.
🏁 Script executed:
#!/bin/bash # Verify the relative import path resolves correctly echo "Checking if the relative import path exists..." ls -la components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/../../../../common/index.js echo "Checking if the common directory structure exists..." find . -name "common" -type d | head -5Length of output: 540
Incorrect relative import path for common module
The import in
components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.tscurrently points four levels up, but thecommon/index.jslives five levels above. Update the path accordingly:• Location:
components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts• Change import from:
- import type { - ClientToServerEvents, - InterServerEvents, - QueryId, - Response, - ServerToClientEvents, - SocketData, - } from "../../../../common/index.js";to:
+ import type { + ClientToServerEvents, + InterServerEvents, + QueryId, + Response, + ServerToClientEvents, + SocketData, + } from "../../../../../common/index.js";This will correctly resolve to
components/log-viewer-webui/common/index.js.Likely an incorrect or invalid review comment.
components/log-viewer-webui/client/src/pages/IngestPage/Details/Files.tsx (1)
1-1: LGTM! Improved null safety implementation.The changes consistently handle nullable values by:
- Adding the
Nullabletype import- Updating the prop type to allow null/undefined values
- Using nullish coalescing (
??) to safely default to 0 before calling.toString()This prevents potential runtime errors and aligns with the nullable handling pattern implemented across other Details components.
Also applies to: 7-7, 20-20
components/log-viewer-webui/client/src/pages/IngestPage/SpaceSavings/index.tsx (2)
40-53: LGTM! Clean refactoring with improved error handling.The refactoring from async/await to promise chains is well-implemented:
- Error handling is improved by logging instead of throwing
- Promise structure is clear and readable
- Proper error context is maintained
56-61: LGTM! Simplified interval management.The change from
useRefto a local variable for interval management is appropriate here since the interval ID is only needed within theuseEffectscope. The initial data load is retained (addressing the previous review question), and cleanup is properly handled.components/log-viewer-webui/client/src/pages/IngestPage/Details/Messages.tsx (1)
1-1: LGTM! Consistent nullable handling pattern.The implementation mirrors the approach used in
Files.tsx, maintaining consistency across the Details components:
- Proper
Nullabletype usage- Safe rendering with nullish coalescing
- Prevents runtime errors from null/undefined values
Also applies to: 7-7, 20-20
components/log-viewer-webui/client/src/pages/IngestPage/Details/sql.ts (3)
1-1: LGTM! Added nullable type support.The
Nullableimport supports the interface changes and aligns with the nullable handling pattern implemented across the Details components.
30-36: LGTM! Improved SQL null handling.The SQL modifications appropriately handle potential null values:
- Removing
NULLIFfor file count is logical sinceCOUNT(DISTINCT ...)naturally returns non-negative integersCOALESCEfor message sum properly handles the case whereSUM()returnsNULL(when no rows exist)- Explicit
CASTtoINTEGERensures proper typing
42-45: LGTM! Interface aligns with SQL query results.Making all fields nullable in the
DetailsIteminterface correctly reflects the potential SQL query results and ensures type safety when consumed by the Details components that now handle nullable values.components/log-viewer-webui/client/src/pages/IngestPage/Details/TimeRange.tsx (2)
8-11: Excellent type safety improvement!The change from nullable to non-nullable Dayjs objects with validity checks is a solid approach that prevents runtime errors and provides consistent behaviour.
22-27: Clean date validation logic.The validity checks for both dates before formatting provide robust error handling and a clear fallback message when timestamp data is unavailable.
components/log-viewer-webui/client/src/pages/IngestPage/Details/index.tsx (3)
26-27: Good type safety improvements with consistent default handling.The changes to use non-nullable Dayjs objects initialized with
dayjs(null)for dates and nullable numbers provide better type consistency across the component hierarchy.Also applies to: 40-43
50-64: Promise chain implementation is acceptable.The refactoring from async/await to promise chains works correctly and maintains the same error handling behaviour.
67-73: Simplified interval management looks good.Removing the useRef and using a local variable for the interval ID makes the code cleaner and easier to follow.
components/log-viewer-webui/client/src/pages/IngestPage/Jobs/index.tsx (2)
54-69: Consistent refactoring with promise chains.The promise-based approach is consistent with the Details component refactoring and maintains proper error handling.
72-78: Clean interval management.The simplified approach using local variables instead of useRef makes the code more readable and follows the same pattern established in other components.
components/log-viewer-webui/client/vite.config.ts (3)
1-1: LGTM! Appropriate import addition.The path import is correctly added to support the new alias configuration.
14-18: LGTM! Manual alias mapping is a good solution.The manual alias configuration properly replaces the removed
vite-tsconfig-pathsplugin functionality. This approach provides explicit control over path resolution.
28-31: LGTM! API proxy configuration supports server integration.The new proxy rule for
/api/correctly forwards requests to the server at localhost:3000, consistent with existing proxy rules and supporting the new Fastify API routes mentioned in the PR objectives.components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/useSearchResults.ts (1)
12-30: LGTM! Well-structured search results hook.The implementation correctly:
- Prevents unnecessary queries when no active search exists
- Uses proper dependency array for the cursor
- Follows React hook patterns consistently
- Integrates well with the search state management
The empty filter
{}and options{}are appropriate for retrieving all search results.components/log-viewer-webui/client/src/pages/SearchPage/SearchState/useResultsMetadata.ts (1)
14-49: LGTM! Robust metadata fetching hook.The implementation demonstrates good practices:
- Efficient querying with specific
_idandlimit: 1- Proper null handling for various edge cases
- Configuration-driven collection naming via settings
- Correct dependency management for the cursor
The multiple null checks ensure robustness when metadata is unavailable or still loading.
components/log-viewer-webui/client/src/pages/SearchPage/SearchState/useUpdateStateWithMetadata.ts (1)
14-30: LGTM! Clean state management for search completion.The hook correctly:
- Monitors metadata changes reactively
- Updates UI state when receiving the completion signal
- Handles null metadata gracefully with early return
- Uses proper dependency array for the effect
The signal-based state transition pattern is well-implemented and integrates cleanly with the search lifecycle.
components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/QueryInput/index.tsx (1)
23-70: Component integration looks solid.The QueryInput component properly integrates with the search store and provides good user feedback through the progress bar. The disabled state logic and query string updates are well implemented.
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/typings.tsx (2)
11-19: Interface changes look well-structured.The updated
SearchResultinterface properly reflects the MongoDB document structure with_idas a string and the addition of original file metadata fields.
46-50: Good addition of the padding constant.The
TABLE_BOTTOM_PADDINGconstant improves maintainability and addresses the previous review comment about extracting magic numbers.components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/index.tsx (2)
27-44: Excellent dynamic height calculation implementation.The height calculation logic properly handles window resizing and uses the extracted padding constant. The cleanup of event listeners is correctly implemented.
23-23: Good integration with the new search results hook.The component properly consumes dynamic search results through the
useSearchResultshook, replacing the previous static data approach.components/log-viewer-webui/client/src/components/QueryBox/index.tsx (2)
27-27: Good integration with Ant Design design tokens.Using
theme.useToken()for accessing design tokens likeborderRadiusLGimproves consistency with the design system.
26-47: Component refactoring improves clarity and structure.The simplified DOM structure, updated prop naming (
inputPropsinstead ofrest), and direct progress rendering without inline styles make the component cleaner and more maintainable.
| * Fetches jobs stats from the server. | ||
| * | ||
| * @throws {Error} If the response is undefined. | ||
| */ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Update JSDoc to reflect actual error handling.
Similar to the Details component, the JSDoc comment mentions @throws {Error} but the implementation catches errors internally instead of throwing to the caller.
/**
* Fetches jobs stats from the server.
- *
- * @throws {Error} If the response is undefined.
*/📝 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.
| * Fetches jobs stats from the server. | |
| * | |
| * @throws {Error} If the response is undefined. | |
| */ | |
| /** | |
| * Fetches jobs stats from the server. | |
| */ |
🤖 Prompt for AI Agents
In components/log-viewer-webui/client/src/pages/IngestPage/Jobs/index.tsx around
lines 50 to 53, the JSDoc comment incorrectly states that the function throws an
Error if the response is undefined, but the implementation actually catches
errors internally and does not throw them to the caller. Update the JSDoc to
remove the @throws tag or revise it to accurately describe the internal error
handling behavior instead of indicating that errors are thrown.
| { | ||
| dataIndex: "timestamp", | ||
| key: "timestamp", | ||
| render: (timestamp: number) => dayjs(timestamp).format(DATETIME_FORMAT_TEMPLATE), |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Verify timestamp format and add error handling.
The render function assumes the timestamp number is valid for dayjs parsing. Consider adding validation to handle potential invalid timestamps gracefully.
- render: (timestamp: number) => dayjs(timestamp).format(DATETIME_FORMAT_TEMPLATE),
+ render: (timestamp: number) => {
+ const date = dayjs(timestamp);
+ return date.isValid() ? date.format(DATETIME_FORMAT_TEMPLATE) : 'Invalid Date';
+ },📝 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.
| render: (timestamp: number) => dayjs(timestamp).format(DATETIME_FORMAT_TEMPLATE), | |
| render: (timestamp: number) => { | |
| const date = dayjs(timestamp); | |
| return date.isValid() ? date.format(DATETIME_FORMAT_TEMPLATE) : 'Invalid Date'; | |
| }, |
🤖 Prompt for AI Agents
In
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/typings.tsx
at line 28, the render function directly formats the timestamp without
validating it. Add a check to verify if the timestamp is a valid number and can
be parsed by dayjs. If invalid, return a fallback string or handle the error
gracefully to avoid runtime issues.
| <Table<SearchResult> | ||
| columns={searchResultsTableColumns} | ||
| pagination={false} | ||
| rowKey={(record) => record._id.toString()} |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Simplify rowKey since _id is already a string.
The toString() call is unnecessary since _id is defined as a string in the interface.
- rowKey={(record) => record._id.toString()}
+ rowKey={(record) => record._id}📝 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.
| rowKey={(record) => record._id.toString()} | |
| rowKey={(record) => record._id} |
🤖 Prompt for AI Agents
In
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/index.tsx
at line 53, remove the unnecessary toString() call on record._id in the rowKey
prop since _id is already a string. Change rowKey to directly return record._id
without calling toString().
| <Progress | ||
| className={styles["queryBoxProgress"] || ""} | ||
| percent={progress ?? 0} | ||
| className={styles["progressBar"] || ""} |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Simplify className assignment.
The || "" fallback is unnecessary since CSS modules should always provide a value for defined classes.
- className={styles["progressBar"] || ""}
+ className={styles["progressBar"]}📝 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.
| className={styles["progressBar"] || ""} | |
| className={styles["progressBar"]} |
🤖 Prompt for AI Agents
In components/log-viewer-webui/client/src/components/QueryBox/index.tsx at line
38, remove the unnecessary fallback `|| ""` from the className assignment since
CSS modules guarantee a defined value for the class. Simply assign
className={styles["progressBar"]} without the fallback.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/index.tsx (1)
51-51: Remove unnecessary toString() call.Based on the interface definition,
_idis already a string, making thetoString()call redundant.- rowKey={(record) => record._id.toString()} + rowKey={(record) => record._id}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/index.tsx(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/useSearchResults.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/useSearchResults.tscomponents/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/index.tsx
🧬 Code Graph Analysis (1)
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/useSearchResults.ts (3)
components/log-viewer-webui/client/src/api/socket/useCursor.tsx (1)
useCursor(79-79)components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/typings.tsx (1)
SearchResult(52-52)components/log-viewer-webui/client/src/pages/SearchPage/SearchState/index.tsx (1)
SEARCH_STATE_DEFAULT(87-87)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (2)
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/index.tsx (2)
23-44: Excellent implementation of dynamic height calculation.The dynamic height calculation with proper event listener cleanup is well-implemented. The use of
useEffectwith an empty dependency array and proper cleanup function prevents memory leaks.
29-44: 🧹 Nitpick (assertive)Add error handling for getBoundingClientRect.
While the null check for
containerRef.currentis good, consider adding error handling for thegetBoundingClientRect()call which could potentially throw in edge cases.const updateHeight = () => { if (containerRef.current) { - const {top} = containerRef.current.getBoundingClientRect(); - const availableHeight = window.innerHeight - top - TABLE_BOTTOM_PADDING; - setTableHeight(availableHeight); + try { + const {top} = containerRef.current.getBoundingClientRect(); + const availableHeight = window.innerHeight - top - TABLE_BOTTOM_PADDING; + setTableHeight(Math.max(0, availableHeight)); // Ensure non-negative height + } catch (error) { + console.warn('Failed to calculate table height:', error); + } } };Likely an incorrect or invalid review comment.
| /** | ||
| * Custom hook to get search results for the current searchJobId. | ||
| * | ||
| * @return | ||
| */ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Complete the JSDoc documentation.
The @return tag is missing its description. This affects code maintainability and developer experience.
/**
* Custom hook to get search results for the current searchJobId.
*
- * @return
+ * @return The cursor object providing reactive access to search results, or null if no active search.
*/📝 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.
| /** | |
| * Custom hook to get search results for the current searchJobId. | |
| * | |
| * @return | |
| */ | |
| /** | |
| * Custom hook to get search results for the current searchJobId. | |
| * | |
| * @return The cursor object providing reactive access to search results, or null if no active search. | |
| */ |
🤖 Prompt for AI Agents
In
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/useSearchResults.ts
around lines 7 to 11, the JSDoc comment for the custom hook is incomplete
because the @return tag lacks a description. Add a clear and concise description
to the @return tag explaining what the hook returns, such as the type and
purpose of the returned search results data or object.
| () => { | ||
| // If there is no active search job, there are no results to fetch. The cursor will | ||
| // return null. | ||
| if (searchJobId === SEARCH_STATE_DEFAULT.searchJobId) { | ||
| return null; | ||
| } | ||
|
|
||
| const collection = new MongoCollectionSocket(searchJobId.toString()); | ||
| return collection.find({}, {}); | ||
| }, | ||
| [searchJobId] | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider optimizing socket creation and add error handling.
The current implementation creates a new MongoCollectionSocket instance on every useCursor factory function call, which could be inefficient. Additionally, there's no error handling for potential socket creation failures.
Consider memoizing the socket creation or adding error boundaries:
const searchResultsCursor = useCursor<SearchResult>(
() => {
// If there is no active search job, there are no results to fetch. The cursor will
// return null.
if (searchJobId === SEARCH_STATE_DEFAULT.searchJobId) {
return null;
}
- const collection = new MongoCollectionSocket(searchJobId.toString());
- return collection.find({}, {});
+ try {
+ const collection = new MongoCollectionSocket(searchJobId.toString());
+ return collection.find({}, {});
+ } catch (error) {
+ console.error('Failed to create MongoCollectionSocket:', error);
+ return null;
+ }
},
[searchJobId]
);📝 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.
| () => { | |
| // If there is no active search job, there are no results to fetch. The cursor will | |
| // return null. | |
| if (searchJobId === SEARCH_STATE_DEFAULT.searchJobId) { | |
| return null; | |
| } | |
| const collection = new MongoCollectionSocket(searchJobId.toString()); | |
| return collection.find({}, {}); | |
| }, | |
| [searchJobId] | |
| ); | |
| const searchResultsCursor = useCursor<SearchResult>( | |
| () => { | |
| // If there is no active search job, there are no results to fetch. The cursor will | |
| // return null. | |
| if (searchJobId === SEARCH_STATE_DEFAULT.searchJobId) { | |
| return null; | |
| } | |
| try { | |
| const collection = new MongoCollectionSocket(searchJobId.toString()); | |
| return collection.find({}, {}); | |
| } catch (error) { | |
| console.error('Failed to create MongoCollectionSocket:', error); | |
| return null; | |
| } | |
| }, | |
| [searchJobId] | |
| ); |
🤖 Prompt for AI Agents
In
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/useSearchResults.ts
around lines 16 to 27, the code creates a new MongoCollectionSocket instance on
every call, which is inefficient and lacks error handling. Refactor to memoize
the MongoCollectionSocket instance so it is only created when searchJobId
changes, and add try-catch blocks or error handling logic around the socket
creation and usage to gracefully handle any failures.
| const collection = new MongoCollectionSocket(searchJobId.toString()); | ||
| return collection.find({}, {}); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clarify the empty query and options parameters.
The find({}, {}) call uses empty objects which may not be immediately clear to other developers. Consider using named constants or comments for better readability.
- const collection = new MongoCollectionSocket(searchJobId.toString());
- return collection.find({}, {});
+ const collection = new MongoCollectionSocket(searchJobId.toString());
+ // Fetch all results with default options
+ const query = {};
+ const options = {};
+ return collection.find(query, options);📝 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 collection = new MongoCollectionSocket(searchJobId.toString()); | |
| return collection.find({}, {}); | |
| const collection = new MongoCollectionSocket(searchJobId.toString()); | |
| // Fetch all results with default options | |
| const query = {}; | |
| const options = {}; | |
| return collection.find(query, options); |
🤖 Prompt for AI Agents
In
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/useSearchResults.ts
around lines 23 to 24, the find method is called with empty objects as query and
options parameters, which can be unclear. Replace the empty objects with named
constants that clearly indicate they represent an empty query and default
options, or add inline comments explaining that these parameters mean no
filtering and default options, to improve code readability.
| dataSource={searchResults ? | ||
| searchResults : | ||
| []}/> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Simplify the ternary operator using nullish coalescing.
The current ternary expression can be simplified for better readability and to handle nullish values more elegantly.
- dataSource={searchResults ?
- searchResults :
- []}/>
+ dataSource={searchResults ?? []}/>📝 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.
| dataSource={searchResults ? | |
| searchResults : | |
| []}/> | |
| dataSource={searchResults ?? []}/> |
🤖 Prompt for AI Agents
In
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/index.tsx
around lines 54 to 56, replace the ternary operator checking if searchResults
exists with the nullish coalescing operator. Change the expression from
"searchResults ? searchResults : []" to "searchResults ?? []" to simplify the
code and handle null or undefined values more cleanly.
| num_files: Nullable<number>; | ||
| num_messages: Nullable<number>; |
There was a problem hiding this comment.
I believe num_files and num_messages will not be null in the latest SQL query, but it's fine to be cautious and this may soon change as we implement the widget system
| void fetchJobsStats(); | ||
| intervalIdRef.current = setInterval(fetchJobsStats, refreshInterval); | ||
| fetchJobsStats(); | ||
| const intervalId = setInterval(fetchJobsStats, refreshInterval); |
| return; | ||
| } | ||
|
|
||
| handleClearResults(); |
There was a problem hiding this comment.
If handleClearResults does not resolve the clearQueryResults() Promise before returning, it's technically possible for clearQueryResults() to run after submitQuery() resolves, right? I still think marking those functions as async + use await / try-catch syntax could have avoided the issue.
... if they are made async, they need to be wrapped before they are called in react component in a non-async wrapper function
React event handlers (e.g., onClick) can be async. I agree it's a bit of hassle that an useEffect hook cannot be async and to call an async function side, we have to either do https://developer.mozilla.org/en-US/docs/Glossary/IIFE or use the .then().catch() syntax
There was a problem hiding this comment.
Their ordering dosent really matter, and it may actually make the UI more responsive. We eventually want to clear the results, but which resolves first is not an issue
junhaoliao
left a comment
There was a problem hiding this comment.
all good except one concurrency issue that is worth discussing: https://github.com/y-scope/clp/pull/931/files#r2114452418
| cancelQuery( | ||
| payload | ||
| ).then(() => { | ||
| console.log("Query cancelled successfully"); | ||
| }) | ||
| .catch((err: unknown) => { | ||
| console.error("Failed to cancel query:", err); | ||
| }); | ||
|
|
||
| store.updateSearchUiState(SEARCH_UI_STATE.DONE); |
There was a problem hiding this comment.
| cancelQuery( | |
| payload | |
| ).then(() => { | |
| console.log("Query cancelled successfully"); | |
| }) | |
| .catch((err: unknown) => { | |
| console.error("Failed to cancel query:", err); | |
| }); | |
| store.updateSearchUiState(SEARCH_UI_STATE.DONE); | |
| cancelQuery(payload) | |
| .then(() => { | |
| console.log("Query cancelled successfully"); | |
| store.updateSearchUiState(SEARCH_UI_STATE.DONE); | |
| }) | |
| .catch((err: unknown) => { | |
| console.error("Failed to cancel query:", err); | |
| }); |
There was a problem hiding this comment.
ditto here. I did this explicitly to make UI more responsive. Another query can be submitted while the previous is being canceled
There was a problem hiding this comment.
as discussed offline, if to mark the state as DONE before the request is handled, we want to place the updateSearchUiState() call before the API call
| cancelQuery( | |
| payload | |
| ).then(() => { | |
| console.log("Query cancelled successfully"); | |
| }) | |
| .catch((err: unknown) => { | |
| console.error("Failed to cancel query:", err); | |
| }); | |
| store.updateSearchUiState(SEARCH_UI_STATE.DONE); | |
| store.updateSearchUiState(SEARCH_UI_STATE.DONE); | |
| cancelQuery(payload) | |
| .then(() => { | |
| console.log("Query cancelled successfully"); | |
| }) | |
| .catch((err: unknown) => { | |
| console.error("Failed to cancel query:", err); | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/search-requests.ts (3)
90-98: State update timing creates potential for inconsistent UI state.Based on the past review discussion, the state is intentionally updated before the API call for UI responsiveness. However, if this is the intended behaviour, the state update should be moved before the API call for clarity.
Following the earlier discussion, if the intent is to update state before the API call for responsiveness:
+ store.updateSearchUiState(SEARCH_UI_STATE.DONE); cancelQuery( payload ).then(() => { console.log("Query cancelled successfully"); }) .catch((err: unknown) => { console.error("Failed to cancel query:", err); }); - - store.updateSearchUiState(SEARCH_UI_STATE.DONE);
102-105: Consider exporting handleClearResults for potential reuse.The
handleClearResultsfunction might be useful in other contexts but is currently only accessible internally.export { + handleClearResults, handleQueryCancel, handleQuerySubmit, };
71-73:⚠️ Potential issueMissing error state handling in query submission.
When
submitQueryfails, the function logs the error but doesn't reset the UI state fromQUERY_ID_PENDING, potentially leaving the UI in an inconsistent state where users cannot submit new queries..catch((err: unknown) => { console.error("Failed to submit query:", err); + store.updateSearchUiState(SEARCH_UI_STATE.DONE); });components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/index.tsx (2)
51-51: Simplify rowKey since _id is already a string.The
toString()call is unnecessary since_idis defined as a string in the interface according to the past comment.- rowKey={(record) => record._id.toString()} + rowKey={(record) => record._id}
54-56: Simplify ternary operator using nullish coalescing.The current ternary expression can be simplified for better readability and to handle nullish values more elegantly.
- dataSource={searchResults ? - searchResults : - []} + dataSource={searchResults ?? []}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/search-requests.ts(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/index.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/index.tsxcomponents/log-viewer-webui/client/src/pages/SearchPage/SearchControls/search-requests.ts
🔇 Additional comments (2)
components/log-viewer-webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/index.tsx (2)
29-44: Good implementation of dynamic height calculation.The
useEffecthook properly handles window resize events and calculates the table height dynamically. The cleanup function correctly removes the event listener to prevent memory leaks.
23-25: Well-structured state management integration.The component correctly integrates with the new search state system using the
useSearchResultshook and manages local UI state for table height responsively.
| if (null === searchJobId) { | ||
| console.error("Cannot clear results: searchJobId is not set."); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| if (null === aggregationJobId) { | ||
| console.error("Cannot clear results: aggregationJobId is not set."); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Apply coding guideline for null comparisons.
According to the coding guidelines, prefer false == <expression> rather than !<expression>. The current null checks should be updated for consistency.
- if (null === searchJobId) {
+ if (false == (null !== searchJobId)) {
console.error("Cannot clear results: searchJobId is not set.");
return;
}
- if (null === aggregationJobId) {
+ if (false == (null !== aggregationJobId)) {
console.error("Cannot clear results: aggregationJobId is not set.");
return;
}Actually, let me reconsider this - the guideline is about negations, not null checks. The current code is fine as written.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/search-requests.ts
around lines 23 to 33, the current null checks using `null === variable` are
acceptable as per the coding guidelines since the guideline applies to
negations, not null comparisons. No changes are needed; keep the null checks as
they are.
junhaoliao
left a comment
There was a problem hiding this comment.
For the PR title, how about
feat(new-webui): Integrate real-time query results with query state management.
Description
Connects main search components to server using both fastify routes and mongoSocket library.
There is some refactor of existing components as well
We still dont support CLP s but this PR is large enough.
Checklist
breaking change.
Validation performed
I tested a bunch of queries and it looks okay. I was getting a bug where occasionaly if you cancel a query, the next query would stall. I made some minor changes to the server, and it seemed to go away. Anyways, if you can reproduce the stall let me know.
Summary by CodeRabbit