feat(webui): Display search results in Presto UI. #1179
Conversation
|
Warning Rate limit exceeded@davemarco has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 10 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughRefactors SearchResultsTable to delegate rendering to engine-specific virtual tables, adds Presto-specific client components/hooks, relocates shared constants, introduces common Presto types, wraps Presto rows for MongoDB insertion, and ensures server creates a per-search MongoDB collection during Presto search initialisation. Updates Presto plugin to use an enum-based engine check. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as SearchResultsTable (client)
participant Store as Search Store
participant VT1 as SearchResultsVirtualTable
participant VT2 as PrestoResultsVirtualTable
UI->>Store: Read current query engine
alt Engine = Presto
UI->>VT2: Render with tableHeight
VT2->>Store: usePrestoSearchResults() subscribes
VT2->>Store: updateNumSearchResultsTable(count)
else
UI->>VT1: Render with tableHeight
VT1->>Store: useSearchResults() subscribes
VT1->>Store: updateNumSearchResultsTable(count)
end
sequenceDiagram
participant Client as Client (submit Presto search)
participant API as POST /api/presto-search
participant Presto as Presto Client
participant Mongo as MongoDB
Client->>API: Start Presto search
API->>Presto: Submit query
Presto-->>API: Return queryId
API->>Mongo: createCollection(searchJobId)
Mongo-->>API: Collection ready
API-->>Client: 201 Created (job info)
loop Streaming rows
Presto-->>API: Fetch rows
API->>API: prestoRowToObject -> { row: {...} }
API->>Mongo: insert({ row: {...} })
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (7)
components/webui/server/src/routes/api/presto-search/index.ts (2)
59-76: Rows are dropped when data arrives beforesearchJobIdis resolved. Buffer and flush.Current logic returns early and skips inserts if
dataarrives beforestatesetssearchJobId. This can lose initial result batches.Refactor to buffer batches until
queryIdis known, then flush:try { searchJobId = await new Promise<string>((resolve, reject) => { - let isResolved = false; + let isResolved = false; + const pendingBatches: Array<{rows: unknown[]; columns: unknown[]}> = []; Presto.client.execute({ // ... - data: (_, data, columns) => { + data: (_, data, columns) => { request.log.info( `Received ${data.length} rows from Presto query` ); - if (false === isResolved) { - request.log.error( - "Presto data received before searchJobId was resolved; " + - "skipping insert." - ); - - return; - } - if (0 === data.length) { return; } - insertPrestoRowsToMongo( - data, - columns, - searchJobId, - mongoDb - ).catch((err: unknown) => { - request.log.error( - err, - "Failed to insert Presto results into MongoDB" - ); - }); + if (false === isResolved) { + // Buffer until we know the queryId/collection + pendingBatches.push({rows: data, columns}); + return; + } + + insertPrestoRowsToMongo(data, columns, searchJobId, mongoDb) + .catch((err: unknown) => { + request.log.error(err, "Failed to insert Presto results into MongoDB"); + }); }, // ... state: (_, queryId, stats) => { request.log.info({ searchJobId: queryId, state: stats.state, }, "Presto search state updated"); if (false === isResolved) { isResolved = true; - resolve(queryId); + resolve(queryId); + // Flush buffered batches + if (0 < pendingBatches.length) { + void Promise.allSettled( + pendingBatches.map((b) => + insertPrestoRowsToMongo(b.rows, b.columns, queryId, mongoDb) + ) + ).then((results) => { + for (const r of results) { + if ("rejected" === r.status) { + request.log.error(r.reason, "Failed to flush buffered Presto rows to MongoDB"); + } + } + }); + pendingBatches.length = 0; + } } },Also applies to: 77-88, 103-107
90-94: Log failures at error level, not info.Use error severity for failures to aid observability and alerting.
- error: (error) => { - request.log.info(error, "Presto search failed"); + error: (error) => { + request.log.error(error, "Presto search failed");components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/typings.tsx (2)
5-8: Normalise the import path.
".././../../../config"is equivalent to"../../../../config"but noisier. Use the simpler relative path.-import { - CLP_STORAGE_ENGINES, - SETTINGS_STORAGE_ENGINE, -} from ".././../../../config"; +import {CLP_STORAGE_ENGINES, SETTINGS_STORAGE_ENGINE} from "../../../../config";
63-66: Remove unintended leading space in fallback stringThe string literal
" Original file"at
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/typings.tsx
includes a stray leading space, which will render extra whitespace in the UI. Update it to remove the space and, if you need additional spacing, handle that via CSS or layout rather than embedded literal whitespace.
- File: components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/typings.tsx (around line 65)
- " Original file" + "Originalfile"components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts (1)
13-28: Return the promise to enable upstream flow control and testing.Let callers await the submission, chain UI side-effects, or handle errors centrally.
-const handlePrestoQuerySubmit = (payload: PrestoQueryJobCreationSchema) => { - const store = useSearchStore.getState(); - submitQuery(payload) - .then((result) => { - const {searchJobId} = result.data; - store.updateSearchJobId(searchJobId); - console.debug( - "Presto search job created - ", - "Search job ID:", - searchJobId - ); - }) - .catch((err: unknown) => { - console.error("Failed to submit query:", err); - }); -}; +const handlePrestoQuerySubmit = async (payload: PrestoQueryJobCreationSchema) => { + const store = useSearchStore.getState(); + try { + const result = await submitQuery(payload); + const {searchJobId} = result.data; + store.updateSearchJobId(searchJobId); + console.debug("Presto search job created - ", "Search job ID:", searchJobId); + return result; + } catch (err) { + console.error("Failed to submit query:", err); + throw err; + } +};components/webui/server/src/routes/api/presto-search/utils.ts (2)
8-14: Update JSDoc to reflect nested{row: ...}structure.The function now wraps columns under
row, but the doc still describes a flat object. Please update to avoid confusion.Apply this diff:
- * @return An object mapping each column name to its corresponding value from the row. + * @return An object with a single `row` property that maps each column name to its corresponding value.
15-25: Tighten types for better safety and IDE support.Return type is currently a broad
Record<string, unknown>, and the insert result drops the schema generic. Define a concrete doc schema and use it end-to-end.Apply this diff:
@@ -const prestoRowToObject = ( +type PrestoRowDoc = {row: Record<string, unknown>}; + +const prestoRowToObject = ( row: unknown[], columns: {name: string}[] -): Record<string, unknown> => { +): PrestoRowDoc => { const obj: Record<string, unknown> = {}; columns.forEach((col, idx) => { obj[col.name] = row[idx]; }); - return {row: obj}; + return {row: obj}; }; @@ -): Promise<InsertManyResult> => { +): Promise<InsertManyResult<PrestoRowDoc>> => { const collection = mongoDb.collection(searchJobId); const resultDocs = data.map((row) => prestoRowToObject(row, columns)); return collection.insertMany(resultDocs); }Also applies to: 41-45
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts(2 hunks)components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/typings.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/usePrestoSearchResults.ts(1 hunks)components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/SearchResultsVirtualTable.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/index.tsx(2 hunks)components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/typings.tsx(1 hunks)components/webui/server/src/routes/api/presto-search/index.ts(1 hunks)components/webui/server/src/routes/api/presto-search/utils.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/typings.tsxcomponents/webui/server/src/routes/api/presto-search/index.tscomponents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.tscomponents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable.tsxcomponents/webui/server/src/routes/api/presto-search/utils.tscomponents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/usePrestoSearchResults.tscomponents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/typings.tsxcomponents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/SearchResultsVirtualTable.tsx
🧠 Learnings (6)
📓 Common learnings
Learnt from: haiqi96
PR: y-scope/clp#0
File: :0-0
Timestamp: 2025-07-29T14:04:13.769Z
Learning: User haiqi96 requested creating a GitHub issue to document a bug fix from PR #1136, which addressed MySQL compatibility issues with invalid SQL CAST operations in the WebUI component.
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
Learnt from: junhaoliao
PR: y-scope/clp#939
File: components/package-template/src/etc/clp-config.yml:64-64
Timestamp: 2025-06-22T04:01:43.409Z
Learning: The new webui architecture uses Fastify with Pino logging instead of the previous Winston-based logging system that was removed during the webui refactoring.
📚 Learning: 2024-11-21T15:51:33.203Z
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
Applied to files:
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/typings.tsxcomponents/webui/server/src/routes/api/presto-search/index.tscomponents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.tscomponents/webui/server/src/routes/api/presto-search/utils.tscomponents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/typings.tsx
📚 Learning: 2025-05-29T20:33:40.653Z
Learnt from: junhaoliao
PR: y-scope/clp#937
File: components/log-viewer-webui/client/src/AntdApp.tsx:16-24
Timestamp: 2025-05-29T20:33:40.653Z
Learning: In components/log-viewer-webui React codebase: Return type annotations (like `: JSX.Element`) are unnecessary and not preferred for React components in JSX/TSX files.
Applied to files:
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/typings.tsxcomponents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable.tsxcomponents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/SearchResultsVirtualTable.tsx
📚 Learning: 2025-04-08T22:32:05.366Z
Learnt from: davemarco
PR: y-scope/clp#797
File: components/log-viewer-webui/client/src/components/Layout/MainLayout.tsx:2-5
Timestamp: 2025-04-08T22:32:05.366Z
Learning: In this codebase using React Router v7.4.1, components should be imported directly from "react-router" (e.g., `import { Link, Outlet } from "react-router";`) rather than from "react-router-dom" as was common in previous versions of React Router.
Applied to files:
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/index.tsx
📚 Learning: 2025-07-18T20:00:50.288Z
Learnt from: hoophalab
PR: y-scope/clp#1108
File: components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsx:15-15
Timestamp: 2025-07-18T20:00:50.288Z
Learning: In the y-scope/clp React webui client codebase, for Zustand store usage: use `useStore.getState().method` for callbacks since the output is not reactive and doesn't need state as a dependency in the hook, and use `useStore((state) => state.property)` with proper selectors for reactive components that need to re-render when state changes.
Applied to files:
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/usePrestoSearchResults.ts
📚 Learning: 2025-05-09T19:15:26.180Z
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:27-31
Timestamp: 2025-05-09T19:15:26.180Z
Learning: For the MongoDB real-time updates implementation in components/log-viewer-webui/client/src/api/socket, a socket singleton pattern is used where a single shared socket connection is maintained rather than creating multiple connections. The socket lifecycle is managed centrally, with unsubscription handling in the useCursor React hook's cleanup function.
Applied to files:
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/usePrestoSearchResults.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (5)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/typings.tsx (1)
28-29: LGTM – comment typo fix.Documentation reads clearly now.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts (1)
14-20: Good use of Zustand’s getState() for a non-reactive callback.This aligns with the project’s pattern for store method access.
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/index.tsx (1)
49-55: LGTM – clean engine-based delegation.Component stays slim and defers data/columns to specialised tables.
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/SearchResultsVirtualTable.tsx (1)
1-49: LGTM – consistent pattern, store update, and stable row keys.Matches existing WebUI patterns (no explicit return types), updates result count via store, uses a string row key via
toString(). No issues spotted.components/webui/server/src/routes/api/presto-search/utils.ts (1)
36-45: Empty inserts are already guarded againstAll current call sites check for
data.length === 0and return early (seecomponents/webui/server/src/routes/api/presto-search/index.ts:72–80), so no additional guard is needed ininsertPrestoRowsToMongo.
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/index.tsx
Show resolved
Hide resolved
...t/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable.tsx
Show resolved
Hide resolved
...t/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable.tsx
Show resolved
Hide resolved
...onents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/typings.tsx
Outdated
Show resolved
Hide resolved
...onents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/typings.tsx
Outdated
Show resolved
Hide resolved
...onents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/typings.tsx
Show resolved
Hide resolved
...lient/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/usePrestoSearchResults.ts
Outdated
Show resolved
Hide resolved
...lient/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/usePrestoSearchResults.ts
Show resolved
Hide resolved
hoophalab
left a comment
hoophalab
left a comment
There was a problem hiding this comment.
looks good to me. some style comments
...lient/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/usePrestoSearchResults.ts
Outdated
Show resolved
Hide resolved
...i/client/src/pages/SearchPage/SearchResults/SearchResultsTable/SearchResultsVirtualTable.tsx
Show resolved
Hide resolved
...onents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/typings.tsx
Show resolved
Hide resolved
| throw error; | ||
| } | ||
|
|
||
| await mongoDb.createCollection(searchJobId); |
There was a problem hiding this comment.
I know this cannot happen in presto-client-node, but in case we change to a more async client, how about adding this comment:
// In the very unlikely situation where the first data arrives before we create the collection,
// MongoDB automatically creates a collection on the first insert if it doesn't exist.
// In this case, `createCollection` does nothing instead of throwing an error, which is what happens in pymongo.
There was a problem hiding this comment.
I think with the current code the situation you describe is not possible. see line here. Like if the data comes first, then it will just exit early and print a log, like it wont create a collection. I dont think the comment is neccesary since if it is actually possible which i doubt, then it will print error log
There was a problem hiding this comment.
We should do better than only runtime correctness, but I'm fine if we don't add a comment here as it doesn't make the code better and we might forget to update comments in the future.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/presto-search-requests.ts
Outdated
Show resolved
Hide resolved
davemarco
left a comment
davemarco
left a comment
There was a problem hiding this comment.
addresses comments, and added some constants where before we had hardcoded strings as quality improvement
...i/client/src/pages/SearchPage/SearchResults/SearchResultsTable/SearchResultsVirtualTable.tsx
Show resolved
Hide resolved
| throw error; | ||
| } | ||
|
|
||
| await mongoDb.createCollection(searchJobId); |
There was a problem hiding this comment.
I think with the current code the situation you describe is not possible. see line here. Like if the data comes first, then it will just exit early and print a log, like it wont create a collection. I dont think the comment is neccesary since if it is actually possible which i doubt, then it will print error log
There was a problem hiding this comment.
Actionable comments posted: 16
🔭 Outside diff range comments (4)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/SearchResultsVirtualTable/typings.tsx (1)
63-66: Remove unintended leading space in fallback label.The string literal has a leading space:
" Original file". If not intentional for formatting, remove it.Apply:
- " Original file" + "Original file"components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/SearchResultsVirtualTable/useSearchResults.ts (1)
24-26: Avoidconsole.login hooks; guard or use a central logger.Leaving
console.login production UI can be noisy. Either wrap in an environment guard or remove.Apply:
- console.log( - `Subscribing to updates to search results with job ID: ${searchJobId}` - ); + if (false == (process.env.NODE_ENV === "production")) { + // eslint-disable-next-line no-console + console.log(`Subscribing to search results for job ID: ${searchJobId}`); + }components/webui/server/src/routes/api/presto-search/utils.ts (2)
9-16: Update JSDoc@returnto reflect the wrapped document shape.The function now returns
{ [PRESTO_DATA_PROPERTY]: obj }, not the plain mapping object. Keep docs accurate.Apply:
- * @return An object mapping each column name to its corresponding value from the row. + * @return A Mongo-friendly document: { [PRESTO_DATA_PROPERTY]: { <columnName>: value, ... } }.
21-24: Consider validating row/column length alignment.If
row.length !== columns.length, some values will beundefinedor dropped. Add a guard (log or throw) to catch upstream mismatches early.For example:
const prestoRowToObject = ( row: unknown[], columns: {name: string}[] ): Record<string, unknown> => { + if (row.length !== columns.length) { + // Use your logger if available instead of console.warn + // eslint-disable-next-line no-console + console.warn("prestoRowToObject: row/columns length mismatch", { + rowLength: row.length, + columnsLength: columns.length, + columns: columns.map(c => c.name), + }); + } const obj: Record<string, unknown> = {}; columns.forEach((col, idx) => { obj[col.name] = row[idx]; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/typings.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/usePrestoSearchResults.ts(1 hunks)components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/utils.ts(1 hunks)components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/SearchResultsVirtualTable/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/SearchResultsVirtualTable/typings.tsx(3 hunks)components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/SearchResultsVirtualTable/useSearchResults.ts(1 hunks)components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/typings.ts(1 hunks)components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/utils.ts(1 hunks)components/webui/common/index.ts(2 hunks)components/webui/server/src/plugins/app/Presto.ts(3 hunks)components/webui/server/src/routes/api/presto-search/utils.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/usePrestoSearchResults.tscomponents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/typings.tscomponents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/utils.tscomponents/webui/common/index.tscomponents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/utils.tscomponents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/SearchResultsVirtualTable/index.tsxcomponents/webui/server/src/plugins/app/Presto.tscomponents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/SearchResultsVirtualTable/typings.tsxcomponents/webui/server/src/routes/api/presto-search/utils.tscomponents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/typings.tsxcomponents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/SearchResultsVirtualTable/useSearchResults.ts
🧠 Learnings (1)
📚 Learning: 2025-05-09T19:15:26.180Z
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:27-31
Timestamp: 2025-05-09T19:15:26.180Z
Learning: For the MongoDB real-time updates implementation in components/log-viewer-webui/client/src/api/socket, a socket singleton pattern is used where a single shared socket connection is maintained rather than creating multiple connections. The socket lifecycle is managed centrally, with unsubscription handling in the useCursor React hook's cleanup function.
Applied to files:
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/SearchResultsVirtualTable/useSearchResults.ts
🧬 Code Graph Analysis (6)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/usePrestoSearchResults.ts (2)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/typings.tsx (1)
PrestoSearchResult(12-12)components/webui/client/src/pages/SearchPage/SearchState/index.tsx (1)
SEARCH_STATE_DEFAULT(153-153)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/index.tsx (3)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/usePrestoSearchResults.ts (1)
usePrestoSearchResults(48-48)components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/typings.tsx (2)
getPrestoSearchResultsTableColumns(13-13)PrestoSearchResult(12-12)components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/utils.ts (1)
getPrestoSearchResultsTableColumns(29-29)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/utils.ts (2)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/typings.tsx (2)
getPrestoSearchResultsTableColumns(13-13)PrestoSearchResult(12-12)components/webui/common/index.ts (1)
PRESTO_DATA_PROPERTY(139-139)
components/webui/server/src/plugins/app/Presto.ts (2)
components/webui/common/index.ts (1)
CLP_QUERY_ENGINES(138-138)components/webui/client/src/settings.ts (1)
settings(36-36)
components/webui/server/src/routes/api/presto-search/utils.ts (1)
components/webui/common/index.ts (1)
PRESTO_DATA_PROPERTY(139-139)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/typings.tsx (1)
components/webui/common/index.ts (1)
PRESTO_DATA_PROPERTY(139-139)
⏰ 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 (18)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/SearchResultsVirtualTable/typings.tsx (2)
36-43: Sorter implementation is stable and correct.Good job ensuring a deterministic sort by breaking ties with
_id, and using a third sort direction to disable clearing. This will avoid subtle UI inconsistencies.Also applies to: 45-51
35-35: UTC plugin is already initialised
The UTC plugin is imported and extended before any rendering occurs, sodayjs.utc(...)will work at runtime.• components/webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/utils.tsx:
– import utc from "dayjs/plugin/utc";
– dayjs.extend(utc);components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/utils.ts (2)
5-5: Type-only import is a good choice to avoid runtime cycles.Switching to
import type {SearchResult}from the VirtualTable typings prevents a runtime dependency, mitigating circular import risks.
14-18: Engine-based stream ID selection looks correct.Logic is straightforward and pure; no changes requested.
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/SearchResultsVirtualTable/useSearchResults.ts (2)
18-23: Early-return on no active search job is clean and prevents unnecessary cursor work.This prevents subscriptions when the default state is active. Looks good.
28-41: No action needed — comment is accurateThe constant
SEARCH_MAX_NUM_RESULTSis defined as 1000 in both the server and client typings, so the “Retrieve 1k most recent results” comment correctly reflects its value.components/webui/server/src/routes/api/presto-search/utils.ts (1)
26-28: Wrapping underPRESTO_DATA_PROPERTYto avoid_idcollisions is the right move.This aligns server output with client expectations and prevents Mongo
_idconflicts. Nice.components/webui/server/src/plugins/app/Presto.ts (3)
42-45: Logging and decoration look good.Passing
clientOptionsas the first argument tofastify.log.infois aligned with Pino’s recommended usage and aids diagnostics; decorating the instance is straightforward.
37-40: No coercion needed for PrestoPort – it’s already numericThe JSON import (
import … from "../../../settings.json" with {type: "json"}) ensuressettings.PrestoPortis typed as a number, and yoursettings.jsondefines it as the numeric literal8889. You can leave the code as-is.
33-35: Simplify engine check; remove unnecessary castThe
as CLP_QUERY_ENGINEScast isn’t needed sinceCLP_QUERY_ENGINESis a string enum.• components/webui/server/src/plugins/app/Presto.ts (lines 33–35)
- if (CLP_QUERY_ENGINES.PRESTO !== settings.ClpQueryEngine as CLP_QUERY_ENGINES) { + if (CLP_QUERY_ENGINES.PRESTO !== settings.ClpQueryEngine) { return; }components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/typings.ts (1)
1-14: LGTM: Centralized, documented constants.Consolidating shared constants here improves cohesion and avoids duplication across renderers.
components/webui/common/index.ts (2)
138-141: LGTM: Publicly exporting PRESTO_DATA_PROPERTY.Centralising this constant avoids drift between client and server representations.
112-112: Narrow PRESTO_DATA_PROPERTY to a literal type.Adding as const ensures typeof PRESTO_DATA_PROPERTY is the string literal "row", which enables correct typing for the mapped type in PrestoSearchResult.
Apply this diff:
-const PRESTO_DATA_PROPERTY = "row"; +const PRESTO_DATA_PROPERTY = "row" as const;Likely an incorrect or invalid review comment.
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/index.tsx (2)
44-51: Presto VirtualTable plumbing looks goodThe component cleanly composes memoized columns, disables pagination, and applies vertical scroll with the provided height. Typing with PrestoSearchResult is consistent.
28-31: getPrestoSearchResultsTableColumns is already O(columns)The
getPrestoSearchResultsTableColumnsimplementation inutils.tsimmediately returnsObject.keys(data[0].row)—it only inspects the first row to derive column definitions. This operation is linear in the number of columns and does not scan all rows, so there’s no O(rows × columns) concern. No changes required.components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/SearchResultsVirtualTable/index.tsx (3)
38-45: LGTM: virtualized table wiring is sound
- Properly typed
VirtualTable<SearchResult>.- Stable
rowKeyimplementation and vertical scroll height.- Pagination disabled as expected.
- No violations of the “prefer false == expr over !expr” guideline.
Nice, focused component.
6-8: SearchResult export verified
- In
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/SearchResultsVirtualTable/typings.tsx:
- Line 73:
export type { SearchResult };- Line 74:
export { searchResultsTableColumns };No further action required.
43-43: Row key uniqueness confirmed
TheSearchResultinterface defines_idas a non-nullable string, sorecord._id.toString()will always yield a unique string key. No changes are required.
...pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/index.tsx
Show resolved
Hide resolved
...pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/index.tsx
Show resolved
Hide resolved
...pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/index.tsx
Show resolved
Hide resolved
...ges/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/typings.tsx
Outdated
Show resolved
Hide resolved
| export type {PrestoSearchResult}; | ||
| export {getPrestoSearchResultsTableColumns} from "./utils"; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider renaming the file to .ts (no JSX).
This file contains only types; using .ts improves clarity and avoids unnecessary TSX processing.
Would you like me to generate a follow-up patch to rename the file and update imports?
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/typings.tsx
around lines 12-13, this file only exports types and utilities so rename the
file from .tsx to .ts to reflect that it contains no JSX; after renaming, update
all import sites to reference typings.ts (or use type-only imports where
appropriate, e.g. import type {...} from './typings') and run a quick
project-wide search to fix any stale .tsx references; no code changes within the
file are required beyond the filename change, but ensure build/tsconfig include
patterns still capture the new .ts file if you rely on explicit globs.
...nt/src/pages/SearchPage/SearchResults/SearchResultsTable/SearchResultsVirtualTable/index.tsx
Show resolved
Hide resolved
...nt/src/pages/SearchPage/SearchResults/SearchResultsTable/SearchResultsVirtualTable/index.tsx
Show resolved
Hide resolved
.../src/pages/SearchPage/SearchResults/SearchResultsTable/SearchResultsVirtualTable/typings.tsx
Show resolved
Hide resolved
...es/SearchPage/SearchResults/SearchResultsTable/SearchResultsVirtualTable/useSearchResults.ts
Show resolved
Hide resolved
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/utils.ts
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/utils.ts (3)
4-4: MakePrestoSearchResultimport type-only to avoid a runtime cycleThis mirrors the earlier feedback: importing the type as a value can create a runtime cycle with
typings.tsx. Type-only import is erased at build time.-import {PrestoSearchResult} from "./typings"; +import type {PrestoSearchResult} from "./typings";
23-30: UsePRESTO_DATA_PROPERTYfor key extraction and fix the ESLint array-bracket-newline error
- Deriving keys from
data[0].rowhard-codes the property name; use the shared constant instead.- The current
dataIndexarray formatting triggers the pipeline failure: “A linebreak is required before ']'”. Move]to its own line (or keep the array on one line).- return Object.keys(data[0].row) - .map((key) => ({ - dataIndex: [ - PRESTO_DATA_PROPERTY, - key], - key: key, - title: key, - })); + const row = data[0][PRESTO_DATA_PROPERTY] as Record<string, unknown>; + return Object.keys(row) + .map((key) => ({ + dataIndex: [ + PRESTO_DATA_PROPERTY, + key + ], + key: key, + title: key, + }));
16-21: UseinwithPRESTO_DATA_PROPERTYfor the presence check (and keep the undefined-guard), matching codebase styleAvoid checking the property’s value via
typeof ... === "undefined". The intention is to ensure the property exists. Also aligns with the coding guideline preferringfalse == <expr>to!<expr>.- if (0 === data.length || - "undefined" === typeof data[0] || - "undefined" === typeof data[0][PRESTO_DATA_PROPERTY] - ) { + if (0 === data.length || + "undefined" === typeof data[0] || + false == (PRESTO_DATA_PROPERTY in data[0]) + ) { return []; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/utils.ts(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/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/utils.ts
🧬 Code Graph Analysis (1)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/utils.ts (2)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/typings.tsx (2)
getPrestoSearchResultsTableColumns(13-13)PrestoSearchResult(12-12)components/webui/common/index.ts (1)
PRESTO_DATA_PROPERTY(139-139)
🪛 GitHub Actions: clp-lint
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/utils.ts
[error] 27-27: lint:check-no-cpp failed. ESLint error in PrestoResultsVirtualTable/utils.ts: A linebreak is required before ']' (rule @stylistic/array-bracket-newline).
.../pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/utils.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/utils.ts (4)
1-1: Make TableProps a type-only import to avoid a runtime edgePrevents emitting a runtime import for a compile-time type.
-import {TableProps} from "antd"; +import type {TableProps} from "antd";
4-4: Mark PrestoSearchResult as a type-only import to prevent runtime cycletypings.tsx re-exports a value from this utils file; importing the type as type-only avoids a module cycle at runtime.
-import {PrestoSearchResult} from "./typings"; +import type {PrestoSearchResult} from "./typings";
16-21: Use property-existence check with the shared constant; avoid typeof “undefined”Simplifies the guard and follows the project style (false == …). Also aligns with the PRESTO_DATA_PROPERTY abstraction.
- if (0 === data.length || - "undefined" === typeof data[0] || - "undefined" === typeof data[0][PRESTO_DATA_PROPERTY] - ) { + if (0 === data.length || false == (PRESTO_DATA_PROPERTY in data[0])) { return []; }
23-31: Avoid hard-coding “row”; use PRESTO_DATA_PROPERTY for key enumerationThe guard uses PRESTO_DATA_PROPERTY but enumeration still references .row. Use the constant consistently.
- return Object.keys(data[0].row) + const row = data[0][PRESTO_DATA_PROPERTY] as Record<string, unknown>; + return Object.keys(row) .map((key) => ({ dataIndex: [ PRESTO_DATA_PROPERTY, key, ], key: key, title: key, }));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/utils.ts(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/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/utils.ts
🧬 Code Graph Analysis (1)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/utils.ts (2)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/typings.tsx (2)
getPrestoSearchResultsTableColumns(13-13)PrestoSearchResult(12-12)components/webui/common/index.ts (1)
PRESTO_DATA_PROPERTY(139-139)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
.../pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/utils.ts
Show resolved
Hide resolved
.../pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/utils.ts
Show resolved
Hide resolved
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
kirkrodrigues
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
components/webui/server/src/routes/api/presto-search/utils.ts (2)
8-15: Update JSDoc to reflect the wrapped return shapeDoc still states it returns a plain mapping; it now returns a wrapper object.
/** - * Converts a Presto result row (array of values) into an object, using the provided column - * definitions to assign property names. + * Converts a Presto result row (array of values) into a Mongo-ready wrapper object, using the + * provided column definitions to assign property names. * * @param row Array of values representing a single Presto result row. * @param columns Array of column definitions, each containing a `name` property. - * @return An object mapping each column name to its corresponding value from the row. + * @returns A PrestoRowObject whose `row` maps each column name to its corresponding value. */
45-46: Annotate resultDocs for clearer intent and safer refactorsExplicitly typing the insertion payload avoids widening to any and future regressions if prestoRowToObject’s signature changes.
- const resultDocs = data.map((row) => prestoRowToObject(row, columns)); + const resultDocs: PrestoRowObject[] = data.map((row) => prestoRowToObject(row, columns));
♻️ Duplicate comments (4)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/utils.ts (4)
1-1: Import TableProps as a type-only importPrevents emitting a runtime import edge.
-import {TableProps} from "antd"; +import type {TableProps} from "antd";
3-3: Import PrestoSearchResult as a type-only importAvoids creating a runtime cycle with typings.
-import {PrestoSearchResult} from "./typings"; +import type {PrestoSearchResult} from "./typings";
6-11: Complete the JSDoc returns descriptionClarify what the function returns to aid maintainability and tooling.
/** * Generates dynamic columns configuration for Presto query engine. * * @param data Array of Presto search results - * @return + * @returns Columns configuration for Ant Design Table derived from the first result row */
15-20: Harden guard against null/non-object row payloadsPrevents runtime errors if row is null or malformed.
- if (0 === data.length || - "undefined" === typeof data[0] || - "undefined" === typeof data[0].row - ) { + if (0 === data.length || + "undefined" === typeof data[0] || + "undefined" === typeof data[0].row || + null === data[0].row || + "object" !== typeof data[0].row + ) { return []; }
📜 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 settings in your CodeRabbit configuration.
📒 Files selected for processing (4)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/typings.ts(1 hunks)components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/utils.ts(1 hunks)components/webui/common/index.ts(1 hunks)components/webui/server/src/routes/api/presto-search/utils.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/typings.tscomponents/webui/common/index.tscomponents/webui/server/src/routes/api/presto-search/utils.tscomponents/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/utils.ts
🧠 Learnings (1)
📚 Learning: 2024-11-21T15:51:33.203Z
Learnt from: junhaoliao
PR: y-scope/clp#596
File: components/log-viewer-webui/client/src/api/query.js:16-23
Timestamp: 2024-11-21T15:51:33.203Z
Learning: In `components/log-viewer-webui/client/src/api/query.js`, the `ExtractJsonResp` type definition is accurate as-is and does not require modification. When suggesting changes to type definitions, ensure they align with the server-side definitions, referencing the source code if necessary.
Applied to files:
components/webui/server/src/routes/api/presto-search/utils.ts
🧬 Code Graph Analysis (3)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/typings.ts (1)
components/webui/common/index.ts (1)
PrestoRowObject(146-146)
components/webui/server/src/routes/api/presto-search/utils.ts (1)
components/webui/common/index.ts (1)
PrestoRowObject(146-146)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/utils.ts (1)
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/typings.ts (1)
PrestoSearchResult(11-11)
🔇 Additional comments (3)
components/webui/common/index.ts (1)
132-139: LGTM: Shared Presto row wrapper is clear and aligned with MongoDB constraintsThe shared type and export make the wrapper explicit, avoiding top-level _id collisions and keeping client/server in sync.
Also applies to: 146-146
components/webui/server/src/routes/api/presto-search/utils.ts (1)
16-27: LGTM: Return type and shape now match the runtime wrapperprestoRowToObject returning PrestoRowObject with a row wrapper is consistent with the shared common type and prevents _id clashes.
components/webui/client/src/pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/typings.ts (1)
1-1: LGTM: Type-only import avoids runtime cyclesUsing import type for PrestoRowObject keeps the client bundle clean.
...ages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/typings.ts
Outdated
Show resolved
Hide resolved
.../pages/SearchPage/SearchResults/SearchResultsTable/Presto/PrestoResultsVirtualTable/utils.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
components/webui/server/src/routes/api/presto-search/index.ts (1)
227-228: Make DELETE /results idempotent; ignore “NamespaceNotFound”.Dropping a non-existent collection can throw and cause a 500. Treat it as success and keep returning 204.
Apply:
- await mongoDb.collection(searchJobId).drop(); + try { + const wasDropped = await mongoDb.collection(searchJobId).drop(); + if (false === wasDropped) { + request.log.warn({searchJobId}, "MongoDB collection did not exist; continuing"); + } + } catch (err: unknown) { + const e = err as { codeName?: string; code?: number }; + // Ignore "NamespaceNotFound" (code: 26) to keep the operation idempotent + if ("NamespaceNotFound" === e.codeName || 26 === e.code) { + request.log.warn({searchJobId}, "MongoDB collection did not exist; continuing"); + } else { + request.log.error(err, "Failed to drop MongoDB collection for Presto search results"); + throw err; + } + }
♻️ Duplicate comments (2)
components/webui/server/src/routes/api/presto-search/index.ts (1)
171-172: Make Mongo collection creation idempotent; handle “NamespaceExists” and keep 201.There is a race window where the first insert can auto-create the collection before this
createCollectionruns, causing a “NamespaceExists” error and a 500. Make the creation defensive and non-fatal for the “already exists” case.Apply:
- await mongoDb.createCollection(searchJobId); + try { + await mongoDb.createCollection(searchJobId); + } catch (err: unknown) { + const e = err as { codeName?: string; code?: number }; + // Ignore "collection already exists" (codeName: NamespaceExists, code: 48) + if ("NamespaceExists" === e.codeName || 48 === e.code) { + request.log.warn({searchJobId}, "MongoDB collection already exists; continuing"); + } else { + request.log.error(err, "Failed to create MongoDB collection for Presto search results"); + throw err; + } + }Also consider validating
searchJobIdagainst Mongo’s collection name rules (length, not empty, not starting withsystem., no NUL) since it originates from an external system.components/webui/common/index.ts (1)
132-139: Make PrestoRowObject generic to preserve row shape end-to-end.Allows callers to strongly type the contained row while preserving current behaviour via a default.
/** * Presto row wrapped in a `row` property to prevent conflicts with MongoDB's `_id` field. */ -interface PrestoRowObject { - row: Record<string, unknown>; -} +interface PrestoRowObject<T extends Record<string, unknown> = Record<string, unknown>> { + row: T; +}
📜 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 settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
components/webui/common/index.ts(1 hunks)components/webui/server/src/routes/api/presto-search/index.ts(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/server/src/routes/api/presto-search/index.tscomponents/webui/common/index.ts
| interface PrestoSearchResult extends PrestoRowObject { | ||
| _id: string; |
There was a problem hiding this comment.
sorry, but can we also move interface PrestoSearchResult to common?
Because it is the type of objects stored in the mongodb.
If someone needs to understand our types in the protocol between server and client, he/she only needs to take a look at the common folder
There was a problem hiding this comment.
and there is a lint error in cicd
hoophalab
left a comment
hoophalab
left a comment
There was a problem hiding this comment.
Validations:
- Run several sql queries
- The search results and column titles are correctly shown in the search results table
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
Deferring to @hoophalab's review.
3 participants
Description
PR pulls results from mongo using mongo socket lib and displays in antd virtual table
Checklist
breaking change.
Validation performed
Results appear in table
Summary by CodeRabbit
New Features
Bug Fixes
Refactor