feat(new-webui): Port search APIs from Meteor methods to Fastify routes.#913
Conversation
## Walkthrough
This change removes the Fastify plugin for `SearchResultsMetadataCollection`, deletes a sample route, and introduces a new search API with supporting utility and schema modules. It also updates TypeScript typings, adjusts error handling, and refines ESLint and type definitions for search results metadata.
## Changes
| File(s) | Change Summary |
|-------------------------------------------------------------------------------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| .../server/src/fastify-v2/plugins/app/search/SearchResultsMetadataCollection/index.ts | Removed the Fastify plugin and related type augmentations for exposing the SearchResultsMetadataCollection. |
| .../server/src/fastify-v2/routes/sample.ts | Deleted the sample Fastify route plugin for `/Example`. |
| .../server/src/fastify-v2/routes/api/search/index.ts | Added a new Fastify plugin implementing `/api/search` endpoints for query, results deletion, and cancellation, managing job submission and MongoDB metadata. |
| .../server/src/fastify-v2/routes/api/search/utils.ts | Added utility functions for checking collections, updating search results metadata, monitoring job completion, and creating MongoDB indexes. |
| .../server/src/fastify-v2/routes/api/search/typings.ts | Added TypeScript typings and constants for search API operations, including metadata update and job monitoring parameters. |
| .../server/src/fastify-v2/schemas/common.ts | Added common TypeBox JSON schemas for strings and IDs. |
| .../server/src/fastify-v2/schemas/error.ts | Added a TypeBox JSON schema for API error responses. |
| .../server/src/fastify-v2/schemas/search.ts | Added TypeBox JSON schemas for search job creation and job identification. |
| .../server/src/fastify-v2/app.ts | Replaced a local HTTP 500 constant with `StatusCodes.INTERNAL_SERVER_ERROR` from `http-status-codes`. |
| .../server/eslint.config.mjs | Updated ESLint config: added `Type.Null` to exceptions. |
| .../common/searchResultsMetadata.ts | Changed `errorMsg` property type from `Nullable<string>` to `string | null`, removed `Nullable` import, and reformatted `SEARCH_SIGNAL` export. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Client
participant FastifyServer
participant SQLJobManager
participant MongoDB
Client->>FastifyServer: POST /api/search/query
FastifyServer->>SQLJobManager: Submit search & aggregation jobs
SQLJobManager-->>FastifyServer: Return job IDs
FastifyServer->>MongoDB: Ensure collections, insert metadata
FastifyServer-->>Client: Respond with job IDs
FastifyServer->>SQLJobManager: Monitor job completion (async)
SQLJobManager-->>FastifyServer: Notify on completion
FastifyServer->>MongoDB: Update metadata, create indexes
Client->>FastifyServer: DELETE /api/search/results
FastifyServer->>MongoDB: Drop collections
FastifyServer-->>Client: Respond 204
Client->>FastifyServer: POST /api/search/cancel
FastifyServer->>SQLJobManager: Cancel jobs
FastifyServer->>MongoDB: Update metadata as cancelled
FastifyServer-->>Client: Respond 204Possibly related PRs
Suggested reviewers
|
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (20)
components/log-viewer-webui/server/eslint.config.mjs(1 hunks)components/log-viewer-webui/server/settings.json(1 hunks)components/log-viewer-webui/server/src/fastify-v2/app.ts(3 hunks)components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/typings.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchJobCollectionsManager/index.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchJobCollectionsManager/typings.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchResultsMetadataCollection/index.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchResultsMetadataCollection/typings.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/routes/api/search/index.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/routes/api/search/typings.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/routes/api/search/utils.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/schemas/common.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/schemas/error.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/schemas/search.ts(1 hunks)components/log-viewer-webui/server/src/plugins/DbManager.ts(1 hunks)components/log-viewer-webui/server/src/routes/query.ts(1 hunks)components/log-viewer-webui/server/src/typings/DbManager.ts(0 hunks)components/log-viewer-webui/server/src/typings/query.ts(1 hunks)components/log-viewer-webui/server/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- components/log-viewer-webui/server/src/typings/DbManager.ts
🧰 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/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/typings.tscomponents/log-viewer-webui/server/src/fastify-v2/schemas/error.tscomponents/log-viewer-webui/server/src/routes/query.tscomponents/log-viewer-webui/server/src/fastify-v2/schemas/common.tscomponents/log-viewer-webui/server/src/plugins/DbManager.tscomponents/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchJobCollectionsManager/typings.tscomponents/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchResultsMetadataCollection/index.tscomponents/log-viewer-webui/server/src/fastify-v2/schemas/search.tscomponents/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchJobCollectionsManager/index.tscomponents/log-viewer-webui/server/src/typings/query.tscomponents/log-viewer-webui/server/src/fastify-v2/routes/api/search/index.tscomponents/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchResultsMetadataCollection/typings.tscomponents/log-viewer-webui/server/src/fastify-v2/routes/api/search/utils.tscomponents/log-viewer-webui/server/src/fastify-v2/routes/api/search/typings.tscomponents/log-viewer-webui/server/src/fastify-v2/app.tscomponents/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts
🧬 Code Graph Analysis (4)
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchResultsMetadataCollection/index.ts (3)
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts (1)
FastifyInstance(160-162)components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchJobCollectionsManager/index.ts (1)
FastifyInstance(86-88)components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchResultsMetadataCollection/typings.ts (1)
SearchResultsMetadataDocument(32-32)
components/log-viewer-webui/server/src/fastify-v2/schemas/search.ts (1)
components/log-viewer-webui/server/src/fastify-v2/schemas/common.ts (2)
StringSchema(10-10)IdSchema(10-10)
components/log-viewer-webui/server/src/fastify-v2/routes/api/search/index.ts (6)
components/log-viewer-webui/server/src/fastify-v2/schemas/search.ts (2)
CreateQueryJobSchema(25-25)QueryJobSchema(26-26)components/log-viewer-webui/server/src/fastify-v2/schemas/error.ts (1)
ErrorSchema(8-8)components/log-viewer-webui/server/src/fastify-v2/routes/api/search/typings.ts (1)
SEARCH_MAX_NUM_RESULTS(42-42)components/webui/imports/api/search/collections.js (1)
SearchResultsMetadataCollection(15-17)components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchResultsMetadataCollection/typings.ts (1)
SEARCH_SIGNAL(33-33)components/log-viewer-webui/server/src/fastify-v2/routes/api/search/utils.ts (3)
updateSearchSignalWhenJobsFinish(141-141)createMongoIndexes(139-139)updateSearchResultsMeta(140-140)
components/log-viewer-webui/server/src/fastify-v2/routes/api/search/typings.ts (4)
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchResultsMetadataCollection/typings.ts (2)
SEARCH_SIGNAL(33-33)SearchResultsMetadataDocument(32-32)components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchResultsMetadataCollection/index.ts (1)
FastifyInstance(26-28)components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchJobCollectionsManager/index.ts (1)
FastifyInstance(86-88)components/log-viewer-webui/server/src/fastify-v2/plugins/app/sample.ts (1)
FastifyInstance(31-33)
🪛 Biome (1.9.4)
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts
[error] 7-7: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (30)
components/log-viewer-webui/server/settings.json (1)
11-11: MongoDB collection configuration added correctlyThe new MongoDB collection name for search results metadata has been properly added to the settings file, maintaining the consistent naming pattern with other MongoDB collection settings.
components/log-viewer-webui/server/eslint.config.mjs (2)
28-28: Adding Type.Null exception to new-cap ruleThis addition is appropriate for supporting TypeBox imports in the new code.
36-52: New max-len rule configurationThe max-len rule configuration looks reasonable with a 100-character limit. The TODO comment indicates this should eventually be moved to eslint-config-yscope.
components/log-viewer-webui/server/src/routes/query.ts (2)
7-10: Updated imports for job type constantsImport paths have been properly updated to reflect the refactoring that centralizes query job type definitions.
32-32: Coding convention correctly appliedGood job using the
false === expressionpattern instead of the!expressionpattern, which follows the project's coding guidelines.components/log-viewer-webui/server/src/fastify-v2/schemas/error.ts (1)
1-8: Well-structured error schema implementationThe error schema provides a clean, simple structure for consistent error responses across the API. The implementation follows TypeScript best practices with appropriate imports and exports.
components/log-viewer-webui/server/tsconfig.json (1)
21-22: Path alias for settings.json looks good.Adding a path alias for the settings file makes imports cleaner and more maintainable throughout the codebase.
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/typings.ts (1)
1-4: Well-documented polling interval constant.Good use of JSDoc comments and appropriate naming convention for the constant. The 500ms polling interval is a reasonable balance between responsiveness and system load.
components/log-viewer-webui/server/src/fastify-v2/schemas/common.ts (1)
1-10: Well-structured common schema definitions.Creating reusable schema validators is a good practice that ensures consistent validation across API endpoints. Both schemas enforce reasonable constraints:
- StringSchema ensures non-empty strings
- IdSchema ensures positive integers for IDs
components/log-viewer-webui/server/src/plugins/DbManager.ts (1)
13-24: Clean separation of type imports.Good refactoring to separate MongoDB-related types from query-related types, improving code organization by grouping related types together. This separation of concerns enhances maintainability.
components/log-viewer-webui/server/src/fastify-v2/app.ts (5)
10-10: Good use of status code constants.Using named constants from http-status-codes instead of magic numbers improves code readability and maintainability.
17-17: Clear file filtering pattern for autoloading.The regex pattern properly identifies utility and typing files that shouldn't be autoloaded as plugins or routes.
51-52: Appropriate application of ignore pattern.Good implementation of the ignore pattern for application plugins to prevent loading utility and typing files.
59-61: Clean route autoloading configuration.Well-structured autoloading configuration with proper pattern exclusion for routes.
78-80: 🧹 Nitpick (assertive)Consider using equality check pattern from coding guidelines.
While this comparison works correctly, the coding guidelines indicate a preference for
false == <expression>pattern over negation operators.Consider rewriting the condition to match the coding guidelines:
- if ("undefined" !== typeof err.statusCode && - StatusCodes.INTERNAL_SERVER_ERROR as number > err.statusCode - ) { + if ("undefined" !== typeof err.statusCode && + false == (StatusCodes.INTERNAL_SERVER_ERROR as number <= err.statusCode) + ) {Likely an incorrect or invalid review comment.
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchJobCollectionsManager/typings.ts (2)
4-11: Well-structured document interfaceThe
SearchResultsDocumentinterface provides a clear and complete structure for search results. Each field has a specific purpose and type, making it easy to understand and work with the data.
16-21: Properly implemented error classThe
CollectionDroppedErrorclass is well designed, extending the standard Error class with a meaningful error message and custom name. This will help with error identification and handling throughout the application.components/log-viewer-webui/server/src/fastify-v2/schemas/search.ts (2)
10-16: Well-defined request schemaThe
CreateQueryJobSchemacorrectly defines all necessary parameters for creating a query job with appropriate types. Using TypeBox ensures proper validation of incoming requests.
19-22: Appropriate job identification schemaThe
QueryJobSchemaproperly defines the structure for identifying a query job using both search and aggregation job IDs, leveraging the reusableIdSchema.components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchResultsMetadataCollection/typings.ts (2)
10-19: Comprehensive search signal enumerationThe
SEARCH_SIGNALenum effectively captures all the necessary states in the search lifecycle, clearly separating request signals from response signals. This provides a solid foundation for tracking search operation states.
25-30: Well-structured metadata document interfaceThe
SearchResultsMetadataDocumentinterface is appropriately designed with clear field definitions. The optionalnumTotalResultsfield is properly documented, explaining that it's only set when the search job is completed.components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchResultsMetadataCollection/index.ts (2)
15-23: Well-implemented collection creation functionThe function properly checks for database availability and throws a meaningful error if the database is not found. The collection is correctly typed with
SearchResultsMetadataDocumentand uses the collection name from settings.
31-41: Correctly implemented Fastify pluginThe plugin is properly structured using fastify-plugin, with appropriate decoration of the Fastify instance and named export. The implementation follows Fastify plugin best practices.
components/log-viewer-webui/server/src/typings/query.ts (1)
1-72: Code looks well structured and properly typedThis new TypeScript module provides clear enum definitions, constants, and interfaces for query jobs that appropriately extend MySQL row data types. The JSDoc comments and typing are comprehensive.
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchJobCollectionsManager/index.ts (2)
19-83: Well-designed collection manager with proper error handlingThe SearchJobCollectionsManager class effectively manages MongoDB collections for search jobs with appropriate error handling and defensive programming techniques. The implementation uses private class fields and static factory method pattern correctly.
85-101: Fastify plugin implementation follows best practicesThe module correctly extends the Fastify interface and registers the manager as a singleton plugin using fastify-plugin.
components/log-viewer-webui/server/src/fastify-v2/routes/api/search/index.ts (1)
27-118: Search query route implemented with comprehensive workflowThe POST /query endpoint correctly implements a multi-step workflow with proper error handling, schema validation, and non-blocking signal updates.
components/log-viewer-webui/server/src/fastify-v2/routes/api/search/utils.ts (3)
21-39: Clean implementation of metadata updatesThe updateSearchResultsMeta function properly handles metadata updates with appropriate logging and MongoDB operations.
52-100: Good error handling in the signal update functionThe updateSearchSignalWhenJobsFinish function properly handles various error conditions, including specific handling for dropped collections, with appropriate early returns.
110-136: Effective MongoDB index creation implementationThe createMongoIndexes function correctly builds and applies both ascending and descending compound indexes on timestamp and _id fields, which should provide good query performance.
| fastify.delete( | ||
| "/results", | ||
| { | ||
| schema: { | ||
| body: QueryJobSchema, | ||
| response: { | ||
| [StatusCodes.NO_CONTENT]: Type.Null(), | ||
| [StatusCodes.INTERNAL_SERVER_ERROR]: ErrorSchema, | ||
| }, | ||
| tags: ["Search"], | ||
| }, | ||
| }, | ||
| async (request, reply) => { | ||
| const {searchJobId, aggregationJobId} = request.body; | ||
|
|
||
| request.log.info({ | ||
| searchJobId, | ||
| aggregationJobId, | ||
| }, "api/search/results args"); | ||
|
|
||
| try { | ||
| await SearchJobCollectionsManager.dropCollection(searchJobId); | ||
| await SearchJobCollectionsManager.dropCollection(aggregationJobId); | ||
| } catch (err: unknown) { | ||
| const errMsg = "Failed to clear search results"; | ||
| request.log.error( | ||
| { | ||
| err, | ||
| searchJobId, | ||
| aggregationJobId, | ||
| }, | ||
| errMsg | ||
| ); | ||
|
|
||
| return reply.internalServerError(`${errMsg} for searchJobId=${searchJobId}, ` + | ||
| `aggregationJobId=${aggregationJobId}`); | ||
| } | ||
|
|
||
| return reply.code(StatusCodes.NO_CONTENT); | ||
| } | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using template literals instead of string concatenation
The error message uses string concatenation with the + operator which is less readable than template literals.
- return reply.internalServerError(`${errMsg} for searchJobId=${searchJobId}, ` +
- `aggregationJobId=${aggregationJobId}`);
+ return reply.internalServerError(`${errMsg} for searchJobId=${searchJobId}, aggregationJobId=${aggregationJobId}`);🤖 Prompt for AI Agents
In components/log-viewer-webui/server/src/fastify-v2/routes/api/search/index.ts
between lines 123 and 163, the error message in the catch block uses string
concatenation with the + operator. Replace this concatenation with a single
template literal to improve readability and maintain consistency in string
formatting.
| fastify.post( | ||
| "/cancel", | ||
| { | ||
| schema: { | ||
| body: QueryJobSchema, | ||
| response: { | ||
| [StatusCodes.NO_CONTENT]: Type.Null(), | ||
| [StatusCodes.INTERNAL_SERVER_ERROR]: ErrorSchema, | ||
| }, | ||
| tags: ["Search"], | ||
| }, | ||
| }, | ||
| async (request, reply) => { | ||
| const {searchJobId, aggregationJobId} = request.body; | ||
|
|
||
| request.log.info({ | ||
| searchJobId, | ||
| aggregationJobId, | ||
| }, "api/search/cancel args"); | ||
|
|
||
| try { | ||
| await QueryJobsDbManager.submitQueryCancellation(searchJobId); | ||
| await QueryJobsDbManager.submitQueryCancellation(aggregationJobId); | ||
|
|
||
| await updateSearchResultsMeta({ | ||
| fields: { | ||
| lastSignal: SEARCH_SIGNAL.RESP_DONE, | ||
| errorMsg: "Query cancelled before it could be completed.", | ||
| }, | ||
| jobId: searchJobId, | ||
| lastSignal: SEARCH_SIGNAL.RESP_QUERYING, | ||
| logger: request.log, | ||
| searchResultsMetadataCollection: SearchResultsMetadataCollection, | ||
| }); | ||
| } catch (err: unknown) { | ||
| const errMsg = "Failed to submit cancel request"; | ||
| request.log.error( | ||
| { | ||
| err, | ||
| searchJobId, | ||
| aggregationJobId, | ||
| }, | ||
| errMsg | ||
| ); | ||
|
|
||
| return reply.internalServerError(`${errMsg} for searchJobId=${searchJobId}, ` + | ||
| `aggregationJobId=${aggregationJobId}`); | ||
| } | ||
|
|
||
| return reply.code(StatusCodes.NO_CONTENT); | ||
| } | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using template literals instead of string concatenation
Similar to the previous route, the error message uses string concatenation with the + operator which is less readable than template literals.
- return reply.internalServerError(`${errMsg} for searchJobId=${searchJobId}, ` +
- `aggregationJobId=${aggregationJobId}`);
+ return reply.internalServerError(`${errMsg} for searchJobId=${searchJobId}, aggregationJobId=${aggregationJobId}`);📝 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.
| fastify.post( | |
| "/cancel", | |
| { | |
| schema: { | |
| body: QueryJobSchema, | |
| response: { | |
| [StatusCodes.NO_CONTENT]: Type.Null(), | |
| [StatusCodes.INTERNAL_SERVER_ERROR]: ErrorSchema, | |
| }, | |
| tags: ["Search"], | |
| }, | |
| }, | |
| async (request, reply) => { | |
| const {searchJobId, aggregationJobId} = request.body; | |
| request.log.info({ | |
| searchJobId, | |
| aggregationJobId, | |
| }, "api/search/cancel args"); | |
| try { | |
| await QueryJobsDbManager.submitQueryCancellation(searchJobId); | |
| await QueryJobsDbManager.submitQueryCancellation(aggregationJobId); | |
| await updateSearchResultsMeta({ | |
| fields: { | |
| lastSignal: SEARCH_SIGNAL.RESP_DONE, | |
| errorMsg: "Query cancelled before it could be completed.", | |
| }, | |
| jobId: searchJobId, | |
| lastSignal: SEARCH_SIGNAL.RESP_QUERYING, | |
| logger: request.log, | |
| searchResultsMetadataCollection: SearchResultsMetadataCollection, | |
| }); | |
| } catch (err: unknown) { | |
| const errMsg = "Failed to submit cancel request"; | |
| request.log.error( | |
| { | |
| err, | |
| searchJobId, | |
| aggregationJobId, | |
| }, | |
| errMsg | |
| ); | |
| return reply.internalServerError(`${errMsg} for searchJobId=${searchJobId}, ` + | |
| `aggregationJobId=${aggregationJobId}`); | |
| } | |
| return reply.code(StatusCodes.NO_CONTENT); | |
| } | |
| ); | |
| } catch (err: unknown) { | |
| const errMsg = "Failed to submit cancel request"; | |
| request.log.error( | |
| { | |
| err, | |
| searchJobId, | |
| aggregationJobId, | |
| }, | |
| errMsg | |
| ); | |
| return reply.internalServerError( | |
| `${errMsg} for searchJobId=${searchJobId}, aggregationJobId=${aggregationJobId}` | |
| ); | |
| } | |
| return reply.code(StatusCodes.NO_CONTENT); | |
| } | |
| ); |
🤖 Prompt for AI Agents
In components/log-viewer-webui/server/src/fastify-v2/routes/api/search/index.ts
between lines 166 and 217, the error message in the catch block uses string
concatenation with the + operator. Replace this concatenation with a single
template literal to improve readability and maintain consistency with other
parts of the code.
| type UpdateSearchResultsMetaProps = { | ||
| jobId: number; | ||
| lastSignal: SEARCH_SIGNAL; | ||
| searchResultsMetadataCollection: FastifyInstance["SearchResultsMetadataCollection"]; | ||
| logger: FastifyBaseLogger; | ||
| fields: Partial<SearchResultsMetadataDocument>; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Guard against unintended mutation of fields
fields will likely be forwarded to MongoDB’s $set update. Wrapping it in Readonly helps catch accidental in-place mutations at compile-time:
- fields: Partial<SearchResultsMetadataDocument>;
+ fields: Readonly<Partial<SearchResultsMetadataDocument>>;This is a zero-cost change that tightens the contract.
📝 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.
| type UpdateSearchResultsMetaProps = { | |
| jobId: number; | |
| lastSignal: SEARCH_SIGNAL; | |
| searchResultsMetadataCollection: FastifyInstance["SearchResultsMetadataCollection"]; | |
| logger: FastifyBaseLogger; | |
| fields: Partial<SearchResultsMetadataDocument>; | |
| }; | |
| type UpdateSearchResultsMetaProps = { | |
| jobId: number; | |
| lastSignal: SEARCH_SIGNAL; | |
| searchResultsMetadataCollection: FastifyInstance["SearchResultsMetadataCollection"]; | |
| logger: FastifyBaseLogger; | |
| fields: Readonly<Partial<SearchResultsMetadataDocument>>; | |
| }; |
🤖 Prompt for AI Agents
In
components/log-viewer-webui/server/src/fastify-v2/routes/api/search/typings.ts
around lines 17 to 23, the fields property in UpdateSearchResultsMetaProps is
mutable but will be forwarded to MongoDB’s $set update. To prevent accidental
in-place mutations, change the type of fields to
Readonly<Partial<SearchResultsMetadataDocument>>. This enforces immutability at
compile-time without runtime cost.
| async awaitJobCompletion (jobId: number): Promise<void> { | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | ||
| while (true) { | ||
| let rows: QueryJob[]; | ||
| try { | ||
| const [queryRows] = await this.#sqlDbConnPool.query<QueryJob[]>( | ||
| ` | ||
| SELECT ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} | ||
| FROM ${settings.SqlDbQueryJobsTableName} | ||
| WHERE ${QUERY_JOBS_TABLE_COLUMN_NAMES.ID} = ? | ||
| `, | ||
| jobId | ||
| ); | ||
|
|
||
| rows = queryRows; | ||
| } catch (e: unknown) { | ||
| let errorMessage: string; | ||
|
|
||
| if (e instanceof Error) { | ||
| errorMessage = e.message; | ||
| } else { | ||
| errorMessage = String(e); | ||
| } | ||
|
|
||
| throw new Error(`Failed to query status for job ${jobId} - ${errorMessage}`); | ||
| } | ||
| if (0 === rows.length) { | ||
| throw new Error(`Job ${jobId} not found in database.`); | ||
| } | ||
|
|
||
| const status = (rows[0] as QueryJob)[QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS]; | ||
|
|
||
| if (false === QUERY_JOB_STATUS_WAITING_STATES.has(status)) { | ||
| if (QUERY_JOB_STATUS.CANCELLED === status) { | ||
| throw new Error(`Job ${jobId} was cancelled.`); | ||
| } else if (QUERY_JOB_STATUS.SUCCEEDED !== status) { | ||
| throw new Error( | ||
| `Job ${jobId} exited with unexpected status=${status}: ` + | ||
| `${Object.keys(QUERY_JOB_STATUS)[status]}.` | ||
| ); | ||
| } | ||
| break; | ||
| } | ||
|
|
||
| await setTimeout(JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add an upper-bound timeout when awaiting job completion
The current while (true) loop polls indefinitely.
If the job hangs or a DB network partition occurs, the promise never resolves, tying up a Fastify request thread forever and risking memory leaks.
Suggested defensive enhancement:
+ const ABS_TIMEOUT = Date.now() + settings.JobCompletionMaxWaitMillis; // e.g. 10 min
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
while (true) {
@@
await setTimeout(JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS);
+
+ if (Date.now() > ABS_TIMEOUT) {
+ throw new Error(
+ `Timed out waiting for job ${jobId} after ` +
+ `${settings.JobCompletionMaxWaitMillis} ms`,
+ );
+ }
}This change prevents runaway loops and surfaces issues to the caller in a predictable manner.
📝 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.
| async awaitJobCompletion (jobId: number): Promise<void> { | |
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | |
| while (true) { | |
| let rows: QueryJob[]; | |
| try { | |
| const [queryRows] = await this.#sqlDbConnPool.query<QueryJob[]>( | |
| ` | |
| SELECT ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} | |
| FROM ${settings.SqlDbQueryJobsTableName} | |
| WHERE ${QUERY_JOBS_TABLE_COLUMN_NAMES.ID} = ? | |
| `, | |
| jobId | |
| ); | |
| rows = queryRows; | |
| } catch (e: unknown) { | |
| let errorMessage: string; | |
| if (e instanceof Error) { | |
| errorMessage = e.message; | |
| } else { | |
| errorMessage = String(e); | |
| } | |
| throw new Error(`Failed to query status for job ${jobId} - ${errorMessage}`); | |
| } | |
| if (0 === rows.length) { | |
| throw new Error(`Job ${jobId} not found in database.`); | |
| } | |
| const status = (rows[0] as QueryJob)[QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS]; | |
| if (false === QUERY_JOB_STATUS_WAITING_STATES.has(status)) { | |
| if (QUERY_JOB_STATUS.CANCELLED === status) { | |
| throw new Error(`Job ${jobId} was cancelled.`); | |
| } else if (QUERY_JOB_STATUS.SUCCEEDED !== status) { | |
| throw new Error( | |
| `Job ${jobId} exited with unexpected status=${status}: ` + | |
| `${Object.keys(QUERY_JOB_STATUS)[status]}.` | |
| ); | |
| } | |
| break; | |
| } | |
| await setTimeout(JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS); | |
| } | |
| } | |
| async awaitJobCompletion (jobId: number): Promise<void> { | |
| // set an absolute timeout so this loop can’t spin forever | |
| const ABS_TIMEOUT = Date.now() + settings.JobCompletionMaxWaitMillis; // e.g. 10 min | |
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | |
| while (true) { | |
| let rows: QueryJob[]; | |
| try { | |
| const [queryRows] = await this.#sqlDbConnPool.query<QueryJob[]>( | |
| ` | |
| SELECT ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} | |
| FROM ${settings.SqlDbQueryJobsTableName} | |
| WHERE ${QUERY_JOBS_TABLE_COLUMN_NAMES.ID} = ? | |
| `, | |
| jobId | |
| ); | |
| rows = queryRows; | |
| } catch (e: unknown) { | |
| let errorMessage: string; | |
| if (e instanceof Error) { | |
| errorMessage = e.message; | |
| } else { | |
| errorMessage = String(e); | |
| } | |
| throw new Error(`Failed to query status for job ${jobId} - ${errorMessage}`); | |
| } | |
| if (0 === rows.length) { | |
| throw new Error(`Job ${jobId} not found in database.`); | |
| } | |
| const status = (rows[0] as QueryJob)[QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS]; | |
| if (false === QUERY_JOB_STATUS_WAITING_STATES.has(status)) { | |
| if (QUERY_JOB_STATUS.CANCELLED === status) { | |
| throw new Error(`Job ${jobId} was cancelled.`); | |
| } else if (QUERY_JOB_STATUS.SUCCEEDED !== status) { | |
| throw new Error( | |
| `Job ${jobId} exited with unexpected status=${status}: ` + | |
| `${Object.keys(QUERY_JOB_STATUS)[status]}.` | |
| ); | |
| } | |
| break; | |
| } | |
| await setTimeout(JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS); | |
| if (Date.now() > ABS_TIMEOUT) { | |
| throw new Error( | |
| `Timed out waiting for job ${jobId} after ` + | |
| `${settings.JobCompletionMaxWaitMillis} ms` | |
| ); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts
between lines 110 and 156, the awaitJobCompletion method uses an infinite
while(true) loop to poll job status, which can cause indefinite hanging if the
job never completes or the database is unreachable. To fix this, introduce an
upper-bound timeout or maximum retry count for the polling loop. Track elapsed
time or number of attempts and throw a timeout error if the limit is exceeded,
ensuring the promise resolves or rejects predictably and preventing resource
leaks.
| async submitQueryCancellation (jobId: number): Promise<void> { | ||
| await this.#sqlDbConnPool.query( | ||
| `UPDATE ${settings.SqlDbQueryJobsTableName} | ||
| SET ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} = ${QUERY_JOB_STATUS.CANCELLING} | ||
| WHERE ${QUERY_JOBS_TABLE_COLUMN_NAMES.ID} = ? | ||
| AND ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} | ||
| IN (${QUERY_JOB_STATUS.PENDING}, ${QUERY_JOB_STATUS.RUNNING})`, | ||
| jobId, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parameterise status constants to prevent SQL-injection-like surprises
Although QUERY_JOB_STATUS.* are numeric enums, embedding them directly in the SQL string still mixes data with code. Use placeholders for consistency and to prevent future regressions if the enum ever becomes a string:
- SET ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} = ${QUERY_JOB_STATUS.CANCELLING}
+ SET ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} = ?
WHERE ${QUERY_JOBS_TABLE_COLUMN_NAMES.ID} = ?
- AND ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS}
- IN (${QUERY_JOB_STATUS.PENDING}, ${QUERY_JOB_STATUS.RUNNING})`,
- jobId,
+ AND ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} IN (?, ?)`,
+ [
+ QUERY_JOB_STATUS.CANCELLING,
+ jobId,
+ QUERY_JOB_STATUS.PENDING,
+ QUERY_JOB_STATUS.RUNNING,
+ ],This maintains the strong guarantee that all dynamic values are bound variables.
📝 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.
| async submitQueryCancellation (jobId: number): Promise<void> { | |
| await this.#sqlDbConnPool.query( | |
| `UPDATE ${settings.SqlDbQueryJobsTableName} | |
| SET ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} = ${QUERY_JOB_STATUS.CANCELLING} | |
| WHERE ${QUERY_JOBS_TABLE_COLUMN_NAMES.ID} = ? | |
| AND ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} | |
| IN (${QUERY_JOB_STATUS.PENDING}, ${QUERY_JOB_STATUS.RUNNING})`, | |
| jobId, | |
| ); | |
| async submitQueryCancellation (jobId: number): Promise<void> { | |
| await this.#sqlDbConnPool.query( | |
| `UPDATE ${settings.SqlDbQueryJobsTableName} | |
| SET ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} = ? | |
| WHERE ${QUERY_JOBS_TABLE_COLUMN_NAMES.ID} = ? | |
| AND ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} IN (?, ?)`, | |
| [ | |
| QUERY_JOB_STATUS.CANCELLING, | |
| jobId, | |
| QUERY_JOB_STATUS.PENDING, | |
| QUERY_JOB_STATUS.RUNNING, | |
| ], | |
| ); | |
| } |
🤖 Prompt for AI Agents
In
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts
around lines 91 to 99, the SQL query embeds numeric enum constants directly in
the query string, which risks mixing data with code and potential SQL injection
if enums change. Refactor the query to use placeholders for all dynamic values,
including the status constants, and pass them as bound parameters in the query
call to ensure all dynamic data is parameterized safely.
There was a problem hiding this comment.
Actionable comments posted: 22
🔭 Outside diff range comments (1)
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts (1)
1-25:⚠️ Potential issueFix import sorting and duplicate import issues.
The linting pipeline indicates several import-related issues that need to be addressed.
Apply this diff to fix the import issues:
+import type { + Collection, + Db, +} from "mongodb"; import {setTimeout} from "node:timers/promises"; import type {MySQLPromisePool} from "@fastify/mysql"; import {encode} from "@msgpack/msgpack"; import {FastifyInstance} from "fastify"; import fp from "fastify-plugin"; import {ResultSetHeader} from "mysql2"; import settings from "../../../../../../settings.json" with {type: "json"}; import { QUERY_JOB_STATUS, QUERY_JOB_STATUS_WAITING_STATES, QUERY_JOB_TYPE, QUERY_JOBS_TABLE_COLUMN_NAMES, QueryJob, } from "../../../../../typings/query.js"; import {JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS} from "./typings.js"; - -import type { - Collection, - Db, -} from "mongodb"; - -import type {SearchResultsMetadataDocument} from "./typings.js"; +import type {SearchResultsMetadataDocument} from "./typings.js";🧰 Tools
🪛 GitHub Actions: clp-lint
[warning] 1-1: Run autofix to sort these imports! (simple-import-sort/imports)
[error] 24-24: './typings.js' import is duplicated (no-duplicate-imports)
♻️ Duplicate comments (5)
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts (2)
113-124: Previous review comment still applies: Parameterize status constants.The SQL injection concern raised in previous reviews about embedding status constants directly in the SQL string is still valid and should be addressed.
134-172: Previous review comment still applies: Add timeout to job completion polling.The infinite loop timeout issue identified in previous reviews for the
awaitJobCompletionmethod still needs to be addressed to prevent indefinite hanging.components/log-viewer-webui/server/src/fastify-v2/routes/api/search/index.ts (1)
207-208: Use template literals instead of string concatenation.The string concatenation issue identified in previous reviews should be addressed for better readability.
Apply this diff:
- return reply.internalServerError(`${errMsg} for searchJobId=${searchJobId}, ` + - `aggregationJobId=${aggregationJobId}`); + return reply.internalServerError(`${errMsg} for searchJobId=${searchJobId}, aggregationJobId=${aggregationJobId}`);components/log-viewer-webui/server/src/fastify-v2/routes/api/search/typings.ts (2)
31-31: Consider exporting the constant inline for clarity.The previous review comment about exporting the constant inline is still valid and should be addressed for better code clarity.
38-38: Guard against unintended mutation of fields.The previous review comment about making the
fieldsproperty readonly is still valid and should be addressed to prevent accidental mutations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
components/log-viewer-webui/common/searchResultsMetadata.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/app.ts(3 hunks)components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts(2 hunks)components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/typings.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchResultsMetadataCollection/index.ts(0 hunks)components/log-viewer-webui/server/src/fastify-v2/routes/api/search/index.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/routes/api/search/typings.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/routes/api/search/utils.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/routes/sample.ts(0 hunks)components/log-viewer-webui/server/src/fastify-v2/schemas/common.ts(1 hunks)
💤 Files with no reviewable changes (2)
- components/log-viewer-webui/server/src/fastify-v2/routes/sample.ts
- components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchResultsMetadataCollection/index.ts
🧰 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/server/src/fastify-v2/schemas/common.tscomponents/log-viewer-webui/common/searchResultsMetadata.tscomponents/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.tscomponents/log-viewer-webui/server/src/fastify-v2/app.tscomponents/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/typings.tscomponents/log-viewer-webui/server/src/fastify-v2/routes/api/search/index.tscomponents/log-viewer-webui/server/src/fastify-v2/routes/api/search/utils.tscomponents/log-viewer-webui/server/src/fastify-v2/routes/api/search/typings.ts
🪛 GitHub Actions: clp-lint
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts
[warning] 1-1: Run autofix to sort these imports! (simple-import-sort/imports)
[error] 24-24: './typings.js' import is duplicated (no-duplicate-imports)
[error] 32-33: Expected blank line between class members (@stylistic/lines-between-class-members); '#searchResultsMetadataCollection' is defined but never used (no-unused-private-class-members)
[error] 37-37: Missing semicolon (@stylistic/semi)
[warning] 39-39: This line has a length of 104. Maximum allowed is 100 (@stylistic/max-len)
[warning] 44-44: Missing JSDoc @throws declaration (jsdoc/require-throws)
[error] 50-50: Block must not be padded by blank lines (@stylistic/padded-blocks)
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/typings.ts
[error] 1-1: Expected 2 empty lines after import statement not followed by another import (import/newline-after-import)
components/log-viewer-webui/server/src/fastify-v2/routes/api/search/index.ts
[warning] 1-1: Run autofix to sort these imports! (simple-import-sort/imports)
[error] 13-13: Imports must be broken into multiple lines if there are more than 1 elements (import-newlines/enforce); Expected a line break after this opening brace (@stylistic/object-curly-newline); Expected a line break before this closing brace (@stylistic/object-curly-newline)
[error] 20-20: Expected 2 empty lines after import statement not followed by another import (import/newline-after-import)
[error] 33-33: Missing semicolon (@stylistic/semi)
components/log-viewer-webui/server/src/fastify-v2/routes/api/search/utils.ts
[error] 1-1: Too many blank lines at the beginning of file. Max of 0 allowed (@stylistic/no-multiple-empty-lines)
[warning] 2-2: Run autofix to sort these imports! (simple-import-sort/imports)
[error] 8-8: Expected 2 empty lines after import statement not followed by another import (import/newline-after-import)
[error] 17-19: Unexpected any. Specify a different type (@typescript-eslint/no-explicit-any); Unsafe assignment of an any value (@typescript-eslint/no-unsafe-assignment); Unsafe call of a(n) any typed value (@typescript-eslint/no-unsafe-call); Unsafe member access on an any value (@typescript-eslint/no-unsafe-member-access); Unsafe return of a value of type any (@typescript-eslint/no-unsafe-return)
[error] 19-19: Unexpected space(s) after '{' and before '}' (@stylistic/block-spacing, @stylistic/object-curly-spacing)
[warning] 52-118: Missing JSDoc @param "props.mongoDb" declaration and missing or incorrect @param names (jsdoc/require-param, jsdoc/check-param-names)
[error] 92-92: There should be no space before this paren (@stylistic/space-in-parens)
[warning] 93-93: Expected blank line before this statement (@stylistic/padding-line-between-statements)
[warning] 148-148: Run autofix to sort these exports! (simple-import-sort/exports)
components/log-viewer-webui/server/src/fastify-v2/routes/api/search/typings.ts
[warning] 1-1: Run autofix to sort these imports! (simple-import-sort/imports)
[error] 6-8: Imports must not be broken into multiple lines if there are 1 or less elements (import-newlines/enforce); Unexpected line break after this opening brace (@stylistic/object-curly-newline); Unexpected line break before this closing brace (@stylistic/object-curly-newline)
[error] 15-15: Expected 2 empty lines after import statement not followed by another import (import/newline-after-import); There should be no space after '{' (@stylistic/object-curly-spacing); There should be no space before '}' (@stylistic/object-curly-spacing)
[error] 44-54: Expected a semicolon (@stylistic/member-delimiter-style)
[warning] 57-57: Run autofix to sort these exports! (simple-import-sort/exports)
🔇 Additional comments (8)
components/log-viewer-webui/common/searchResultsMetadata.ts (3)
7-16: LGTM! Well-defined enum structure.The SEARCH_SIGNAL enum provides clear categorization of request and response states for search operations, making the API more predictable and type-safe.
22-27: Good explicit typing approach.Using
string | nullinstead of a Nullable type alias improves clarity and reduces dependency on additional type definitions.
30-32: Export formatting is consistent.The multi-line export format improves readability and follows common TypeScript conventions.
components/log-viewer-webui/server/src/fastify-v2/schemas/common.ts (2)
4-6: Solid string validation schema.The StringSchema with minLength constraint effectively prevents empty string inputs, which is essential for API validation.
8-8: Appropriate ID validation.The IdSchema ensures positive integer values, which aligns with typical database ID requirements.
components/log-viewer-webui/server/src/fastify-v2/app.ts (3)
10-10: Good practice using status code constants.Importing StatusCodes improves code maintainability and reduces magic numbers.
17-17: Effective file filtering pattern.The regex pattern correctly excludes utility and typings files from autoloading, preventing them from being treated as plugins.
77-79:Details
✅ Verification successful
Verify error handling logic correctness.
The condition checks if
err.statusCode < 500to use the original status code, otherwise defaults to internal server error. This logic appears sound but should be verified for edge cases.
🏁 Script executed:
#!/bin/bash # Description: Verify error handling patterns in similar Fastify applications # Expected: Confirm this is a standard pattern for error status code handling rg -A 5 -B 5 "StatusCodes\.INTERNAL_SERVER_ERROR.*err\.statusCode|err\.statusCode.*StatusCodes\.INTERNAL_SERVER_ERROR"Length of output: 1034
Error handling logic verified
The condition in
components/log-viewer-webui/server/src/fastify-v2/app.ts(lines 77–79) correctly uses any definederr.statusCodebelow 500 and defaults to500otherwise. This matches Fastify’s typical distinction between client and server errors—no changes needed here.
| enum SEARCH_SIGNAL { | ||
| NONE = "none", | ||
|
|
||
| REQ_CANCELLING = "req-cancelling", | ||
| REQ_CLEARING = "req-clearing", | ||
| REQ_QUERYING = "req-querying", | ||
|
|
||
| RESP_DONE = "resp-done", | ||
| RESP_QUERYING = "resp-querying", | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address code duplication with common module.
The SEARCH_SIGNAL enum is duplicated between this file and components/log-viewer-webui/common/searchResultsMetadata.ts. This creates maintenance overhead and potential inconsistencies.
Consider importing the enum from the common module instead:
-enum SEARCH_SIGNAL {
- NONE = "none",
-
- REQ_CANCELLING = "req-cancelling",
- REQ_CLEARING = "req-clearing",
- REQ_QUERYING = "req-querying",
-
- RESP_DONE = "resp-done",
- RESP_QUERYING = "resp-querying",
-}
+import {SEARCH_SIGNAL} from "../../../../../common/searchResultsMetadata.js";Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/typings.ts
around lines 14 to 23, the SEARCH_SIGNAL enum is duplicated from
components/log-viewer-webui/common/searchResultsMetadata.ts. To fix this, remove
the enum declaration here and instead import SEARCH_SIGNAL from the common
module to avoid duplication and ensure consistency.
| interface SearchResultsMetadataDocument { | ||
| _id: string; | ||
| errorMsg: Nullable<string>; | ||
| lastSignal: SEARCH_SIGNAL; | ||
| numTotalResults?: number; | ||
| } |
There was a problem hiding this comment.
Fix type inconsistency and eliminate duplication.
The SearchResultsMetadataDocument interface is duplicated with a type inconsistency. In the common module, errorMsg is typed as string | null, but here it uses Nullable<string>. This creates potential runtime issues and maintenance problems.
Consider importing the interface from the common module and ensuring consistent typing:
-interface SearchResultsMetadataDocument {
- _id: string;
- errorMsg: Nullable<string>;
- lastSignal: SEARCH_SIGNAL;
- numTotalResults?: number;
-}
+import {SearchResultsMetadataDocument} from "../../../../../common/searchResultsMetadata.js";This also eliminates the need for the Nullable import if no other types in this file require it.
📝 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.
| interface SearchResultsMetadataDocument { | |
| _id: string; | |
| errorMsg: Nullable<string>; | |
| lastSignal: SEARCH_SIGNAL; | |
| numTotalResults?: number; | |
| } | |
| import {SearchResultsMetadataDocument} from "../../../../../common/searchResultsMetadata.js"; |
🤖 Prompt for AI Agents
In
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/typings.ts
around lines 29 to 34, the SearchResultsMetadataDocument interface is duplicated
and uses Nullable<string> for errorMsg, causing type inconsistency with the
common module where errorMsg is string | null. To fix this, remove the local
interface definition and import SearchResultsMetadataDocument directly from the
common module to ensure consistent typing and eliminate duplication. Also,
remove the Nullable import if it is no longer needed elsewhere in the file.
| this.#searchResultsMetadataCollection = this.#mongoDb.collection<SearchResultsMetadataDocument>( | ||
| settings.MongoDbSearchResultsMetadataCollectionName | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix line length and add proper spacing.
The line exceeds the maximum allowed length of 100 characters.
- this.#searchResultsMetadataCollection = this.#mongoDb.collection<SearchResultsMetadataDocument>(
- settings.MongoDbSearchResultsMetadataCollectionName
- );
+ this.#searchResultsMetadataCollection = this.#mongoDb
+ .collection<SearchResultsMetadataDocument>(
+ settings.MongoDbSearchResultsMetadataCollectionName
+ );📝 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.
| this.#searchResultsMetadataCollection = this.#mongoDb.collection<SearchResultsMetadataDocument>( | |
| settings.MongoDbSearchResultsMetadataCollectionName | |
| ); | |
| this.#searchResultsMetadataCollection = this.#mongoDb | |
| .collection<SearchResultsMetadataDocument>( | |
| settings.MongoDbSearchResultsMetadataCollectionName | |
| ); |
🧰 Tools
🪛 GitHub Actions: clp-lint
[warning] 39-39: This line has a length of 104. Maximum allowed is 100 (@stylistic/max-len)
🤖 Prompt for AI Agents
In
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts
around lines 39 to 41, the line assigning this.#searchResultsMetadataCollection
exceeds 100 characters. Break the line into multiple lines to keep each under
100 characters and add proper spacing for readability, such as placing the
collection name argument on a new line and aligning parentheses correctly.
| import { | ||
| FastifyBaseLogger, | ||
| FastifyInstance, | ||
| } from "fastify"; | ||
|
|
||
| import { | ||
| SEARCH_SIGNAL, | ||
| } from "@common/searchResultsMetadata.js"; | ||
|
|
||
| import type { | ||
| Collection, | ||
| Db, | ||
| } from "mongodb"; | ||
|
|
||
| import { Nullable } from "../../../../typings/common.js"; | ||
|
|
There was a problem hiding this comment.
Fix import sorting and formatting issues.
Multiple linting issues need to be addressed in the import section.
Apply this diff to fix the import and formatting issues:
-import {
- FastifyBaseLogger,
- FastifyInstance,
-} from "fastify";
+import {FastifyBaseLogger, FastifyInstance} from "fastify";
+import type {Collection, Db} from "mongodb";
-import {
- SEARCH_SIGNAL,
-} from "@common/searchResultsMetadata.js";
+import {SEARCH_SIGNAL} from "@common/searchResultsMetadata.js";
+import {Nullable} from "../../../../typings/common.js";
-import type {
- Collection,
- Db,
-} from "mongodb";
-import { Nullable } from "../../../../typings/common.js";
+📝 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.
| import { | |
| FastifyBaseLogger, | |
| FastifyInstance, | |
| } from "fastify"; | |
| import { | |
| SEARCH_SIGNAL, | |
| } from "@common/searchResultsMetadata.js"; | |
| import type { | |
| Collection, | |
| Db, | |
| } from "mongodb"; | |
| import { Nullable } from "../../../../typings/common.js"; | |
| import {FastifyBaseLogger, FastifyInstance} from "fastify"; | |
| import type {Collection, Db} from "mongodb"; | |
| import {SEARCH_SIGNAL} from "@common/searchResultsMetadata.js"; | |
| import {Nullable} from "../../../../typings/common.js"; | |
🧰 Tools
🪛 GitHub Actions: clp-lint
[warning] 1-1: Run autofix to sort these imports! (simple-import-sort/imports)
[error] 6-8: Imports must not be broken into multiple lines if there are 1 or less elements (import-newlines/enforce); Unexpected line break after this opening brace (@stylistic/object-curly-newline); Unexpected line break before this closing brace (@stylistic/object-curly-newline)
[error] 15-15: Expected 2 empty lines after import statement not followed by another import (import/newline-after-import); There should be no space after '{' (@stylistic/object-curly-spacing); There should be no space before '}' (@stylistic/object-curly-spacing)
🤖 Prompt for AI Agents
In
components/log-viewer-webui/server/src/fastify-v2/routes/api/search/typings.ts
lines 1 to 16, the import statements have sorting and formatting issues causing
lint errors. Reorder the imports alphabetically by module source, group related
imports together, and ensure consistent spacing and line breaks between import
groups. Also, align the import braces and remove any unnecessary blank lines to
comply with linting rules.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (10)
components/log-viewer-webui/server/src/fastify-v2/routes/api/search/index.ts (5)
1-21: Fix import sorting, formatting, and spacing issues.Multiple linting issues need to be addressed in the import section.
Apply this diff to fix the import and formatting issues:
-import { - FastifyPluginAsyncTypebox, - Type, -} from "@fastify/type-provider-typebox"; +import {FastifyPluginAsyncTypebox, Type} from "@fastify/type-provider-typebox"; import {StatusCodes} from "http-status-codes"; import {SEARCH_SIGNAL} from "@common/searchResultsMetadata.js"; import {ErrorSchema} from "../../../schemas/error.js"; import { CreateQueryJobSchema, QueryJobSchema, } from "../../../schemas/search.js"; -import {SEARCH_MAX_NUM_RESULTS, SearchResultsMetadataDocument} from "./typings.js"; +import { + SEARCH_MAX_NUM_RESULTS, + SearchResultsMetadataDocument, +} from "./typings.js"; import { createMongoIndexes, updateSearchResultsMeta, updateSearchSignalWhenJobsFinish, } from "./utils.js"; + import settings from "../../../../../settings.json" with {type: "json"};🧰 Tools
🪛 GitHub Actions: clp-lint
[error] 1-33: ESLint errors: imports must be broken into multiple lines (line 13), expected line breaks after opening and before closing braces (line 13), expected 2 empty lines after import statement (line 20), missing semicolon (line 33); also warnings for import sorting.
33-33: Add missing semicolon.The linting pipeline indicates a missing semicolon.
- const mongoDb = mongo.db + const mongoDb = mongo.db;🧰 Tools
🪛 GitHub Actions: clp-lint
[error] 1-33: ESLint errors: imports must be broken into multiple lines (line 13), expected line breaks after opening and before closing braces (line 13), expected 2 empty lines after import statement (line 20), missing semicolon (line 33); also warnings for import sorting.
93-94: Add error handling for MongoDB collection creation.The comment indicates collections should not throw errors if they already exist, but it's better to handle potential errors explicitly.
- await mongoDb.createCollection(searchJobId.toString()); - await mongoDb.createCollection(aggregationJobId.toString()); + try { + // Should not throw an error if the collection already exists. + await mongoDb.createCollection(searchJobId.toString()); + await mongoDb.createCollection(aggregationJobId.toString()); + } catch (err: unknown) { + // Collection might already exist, which is fine + request.log.debug(err, "Collection creation failed (might already exist)"); + }
152-153: Add error handling for collection drop operations.The collection drop operations should include error handling in case the collections don't exist.
- await mongoDb.collection(searchJobId.toString()).drop(); - await mongoDb.collection(aggregationJobId.toString()).drop(); + try { + await mongoDb.collection(searchJobId.toString()).drop(); + await mongoDb.collection(aggregationJobId.toString()).drop(); + } catch (err: unknown) { + const errMsg = "Failed to drop search result collections"; + request.log.error(err, errMsg); + return reply.internalServerError(`${errMsg} for searchJobId=${searchJobId}, aggregationJobId=${aggregationJobId}`); + }
205-206: Consider using template literals instead of string concatenationThe error message uses string concatenation with the
+operator which is less readable than template literals.- return reply.internalServerError(`${errMsg} for searchJobId=${searchJobId}, ` + - `aggregationJobId=${aggregationJobId}`); + return reply.internalServerError(`${errMsg} for searchJobId=${searchJobId}, aggregationJobId=${aggregationJobId}`);components/log-viewer-webui/server/src/fastify-v2/routes/api/search/typings.ts (5)
1-15: Fix import sorting and formatting issues.Multiple linting issues need to be addressed in the import section.
Apply this diff to fix the import and formatting issues:
-import { - FastifyBaseLogger, - FastifyInstance, -} from "fastify"; +import {FastifyBaseLogger, FastifyInstance} from "fastify"; +import type {Collection, Db} from "mongodb"; -import { - SEARCH_SIGNAL, - SearchResultsMetadataDocument, -} from "@common/searchResultsMetadata.js"; +import {SEARCH_SIGNAL, SearchResultsMetadataDocument} from "@common/searchResultsMetadata.js"; -import type { - Collection, - Db, -} from "mongodb"; +🧰 Tools
🪛 GitHub Actions: clp-lint
[error] 1-45: ESLint errors: expected 2 empty lines after import (line 14), expected semicolons (lines 32, 42); warnings for import and export sorting.
19-19: Consider exporting the constant inline for clarityDeclaring & exporting in one step makes the intent unmistakable and avoids the risk of the variable being mutated before export.
-const SEARCH_MAX_NUM_RESULTS = 1000; +export const SEARCH_MAX_NUM_RESULTS = 1_000 as const;Using the numeric separator also improves readability.
22-22: Guard against unintended mutation offields
fieldswill likely be forwarded to MongoDB's$setupdate. Wrapping it inReadonlyhelps catch accidental in-place mutations at compile-time:- fields: Partial<SearchResultsMetadataDocument>; + fields: Readonly<Partial<SearchResultsMetadataDocument>>;This is a zero-cost change that tightens the contract.
29-43: Add missing semicolons.The linting pipeline indicates missing semicolons in the type definitions.
type UpdateSearchSignalWhenJobsFinishProps = { aggregationJobId: number; logger: FastifyBaseLogger; - mongoDb: Db, + mongoDb: Db; queryJobsDbManager: FastifyInstance["QueryJobsDbManager"]; searchJobId: number; searchResultsMetadataCollection: Collection<SearchResultsMetadataDocument>; - }; type CreateMongoIndexesProps = { searchJobId: number; logger: FastifyBaseLogger; - mongoDb: Db, + mongoDb: Db; };
45-51: Fix export sorting.The linting pipeline indicates the exports should be sorted alphabetically.
export { CreateMongoIndexesProps, SEARCH_MAX_NUM_RESULTS, - UpdateSearchResultsMetaProps, SearchResultsMetadataDocument, + UpdateSearchResultsMetaProps, UpdateSearchSignalWhenJobsFinishProps, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
components/log-viewer-webui/common/searchResultsMetadata.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/app.ts(2 hunks)components/log-viewer-webui/server/src/fastify-v2/routes/api/search/index.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/routes/api/search/typings.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/server/src/fastify-v2/app.tscomponents/log-viewer-webui/common/searchResultsMetadata.tscomponents/log-viewer-webui/server/src/fastify-v2/routes/api/search/index.tscomponents/log-viewer-webui/server/src/fastify-v2/routes/api/search/typings.ts
🧬 Code Graph Analysis (1)
components/log-viewer-webui/server/src/fastify-v2/routes/api/search/typings.ts (3)
components/log-viewer-webui/common/searchResultsMetadata.ts (2)
SearchResultsMetadataDocument(29-29)SEARCH_SIGNAL(31-31)components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts (1)
FastifyInstance(158-160)components/log-viewer-webui/server/src/fastify-v2/plugins/app/sample.ts (1)
FastifyInstance(31-33)
🪛 GitHub Actions: clp-lint
components/log-viewer-webui/server/src/fastify-v2/routes/api/search/index.ts
[error] 1-33: ESLint errors: imports must be broken into multiple lines (line 13), expected line breaks after opening and before closing braces (line 13), expected 2 empty lines after import statement (line 20), missing semicolon (line 33); also warnings for import sorting.
components/log-viewer-webui/server/src/fastify-v2/routes/api/search/typings.ts
[error] 1-45: ESLint errors: expected 2 empty lines after import (line 14), expected semicolons (lines 32, 42); warnings for import and export sorting.
🔇 Additional comments (4)
components/log-viewer-webui/server/src/fastify-v2/app.ts (1)
10-10:Details
✅ Verification successful
Import
StatusCodesfor standard HTTP status codes
The new import correctly replaces the hard-coded constant with a centralised source. Ensure thathttp-status-codesis declared in yourpackage.jsondependencies.
🏁 Script executed:
#!/bin/bash # Verify that http-status-codes is listed in package.json dependencies grep -R '"http-status-codes"' -n package.jsonLength of output: 93
🏁 Script executed:
#!/bin/bash # Locate all package.json files in the repo find . -type f -name "package.json" # Search for http-status-codes in all files grep -R '"http-status-codes"' -n .Length of output: 921
Import
StatusCodesfor standard HTTP status codes
The new import replaces the hard-coded constant with a centralised source. Verified thathttp-status-codesis declared incomponents/log-viewer-webui/server/package.json.# Confirm dependency in server package.json grep -R '"http-status-codes"' -n components/log-viewer-webui/server/package.json # -> 38: "http-status-codes": "^2.3.0",components/log-viewer-webui/common/searchResultsMetadata.ts (1)
24-24: Good type simplification!Replacing
Nullable<string>with the explicit union typestring | nullimproves code clarity and reduces dependency on custom type aliases.components/log-viewer-webui/server/src/fastify-v2/routes/api/search/index.ts (2)
35-37: Good defensive programming!Checking for undefined MongoDB database before proceeding prevents runtime errors.
103-114: Excellent async pattern for response optimization!Using
setImmediateto defer the signal update ensures the client receives a quick response while background processing continues. The error handling is also properly implemented.
| if ("undefined" !== typeof err.statusCode && | ||
| StatusCodes.INTERNAL_SERVER_ERROR as number > err.statusCode | ||
| ) { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Refactor conditional for readability and remove redundant cast
The current check:
if ("undefined" !== typeof err.statusCode &&
StatusCodes.INTERNAL_SERVER_ERROR as number > err.statusCode
) { … }can be clearer and drop the unnecessary as number cast. Consider rewriting it as:
-if ( "undefined" !== typeof err.statusCode &&
- StatusCodes.INTERNAL_SERVER_ERROR as number > err.statusCode
-) {
+if ( typeof err.statusCode !== "undefined" &&
+ err.statusCode < StatusCodes.INTERNAL_SERVER_ERROR
+) {🤖 Prompt for AI Agents
In components/log-viewer-webui/server/src/fastify-v2/app.ts around lines 75 to
77, refactor the conditional that checks err.statusCode for better readability
by reversing the typeof check to 'typeof err.statusCode !== "undefined"' and
remove the redundant 'as number' cast on StatusCodes.INTERNAL_SERVER_ERROR.
Rewrite the condition to a clearer form without the type assertion.
|
rerequesting @junhaoliao now that i merged main |
| interface SearchResultsMetadataDocument { | ||
| _id: string; | ||
| errorMsg: Nullable<string>; | ||
| errorMsg: string | null; |
There was a problem hiding this comment.
it is equivalent to Nullable<string>?
| errorMsg: string | null; | |
| errorMsg: Nullable<string>; |
There was a problem hiding this comment.
we dont have nullable type in global common directory yet. I want to change the common directory a bit, but it will require changes to the client outside of this PR. so thats why for now i just did | null
There was a problem hiding this comment.
how about
| errorMsg: string | null; | |
| // eslint-disable-next-line no-warning-comments | |
| // TODO: Replace with Nullable<string> when the `@common` directory refactoring is completed. | |
| errorMsg: string | null; |
| export { | ||
| SEARCH_SIGNAL | ||
| }; |
There was a problem hiding this comment.
| export { | |
| SEARCH_SIGNAL | |
| }; | |
| export {SEARCH_SIGNAL}; |
| // eslint-disable-next-line no-warning-comments | ||
| // TODO: update eslint-config-yscope | ||
| "@stylistic/max-len": [ | ||
| "warn", | ||
| { | ||
| code: 100, | ||
| comments: 100, | ||
| ignoreComments: false, | ||
| ignorePattern: "^(import\\s.+\\sfrom\\s.+|\\} from)", | ||
| ignoreRegExpLiterals: true, | ||
| ignoreStrings: false, | ||
| ignoreTemplateLiterals: false, | ||
| ignoreTrailingComments: false, | ||
| ignoreUrls: true, | ||
| tabWidth: 4, | ||
| }, | ||
| ], |
There was a problem hiding this comment.
i suppose this is added to ignore a super long import line? is it still needed in current code?
There was a problem hiding this comment.
i needed it before, but maybe after refactoring from #912, it is actually no longer needed. I will remove
|
|
||
| if ("undefined" !== typeof err.statusCode && INTERNAL_SERVER_ERROR_CODE > err.statusCode) { | ||
| if ("undefined" !== typeof err.statusCode && | ||
| StatusCodes.INTERNAL_SERVER_ERROR as number > err.statusCode |
There was a problem hiding this comment.
would it be better to write
| StatusCodes.INTERNAL_SERVER_ERROR as number > err.statusCode | |
| Number(StatusCodes.INTERNAL_SERVER_ERROR) > err.statusCode |
| searchJobId: searchJobId, | ||
| logger: request.log, | ||
| mongoDb: mongoDb, | ||
|
|
There was a problem hiding this comment.
seems this suggestion has not been applied
|
|
||
|
|
||
| /* Schema for request to create a new query job */ | ||
| const CreateQueryJobSchema = Type.Object({ |
There was a problem hiding this comment.
how about QueryJobCreationSchema?
| } from "./common.js"; | ||
|
|
||
|
|
||
| /* Schema for request to create a new query job */ |
| timestampEnd: Type.Integer(), | ||
| }); | ||
|
|
||
| /* Schema to identify query job */ |
| }); | ||
|
|
||
| /* Schema to identify query job */ | ||
| const QueryJobSchema = Type.Object({ |
There was a problem hiding this comment.
Since there are 2 ids, i think QueryJobId is a bit confusing, and prefer to leave it. lmk
| interface SearchResultsMetadataDocument { | ||
| _id: string; | ||
| errorMsg: Nullable<string>; | ||
| errorMsg: string | null; |
There was a problem hiding this comment.
Is there any specific reason for changing Nullable to | null?
There was a problem hiding this comment.
see other comment
| } catch (err: unknown) { | ||
| const errMsg = "Unable to submit search/aggregation job to the SQL database"; | ||
| request.log.error(err, errMsg); | ||
|
|
||
| return reply.internalServerError(errMsg); | ||
| } |
There was a problem hiding this comment.
Since fastify automatically prints informative debugging information and replies with a 500 error when an error is uncaught, I personally feel we could just don't catch error at all in routes,
There was a problem hiding this comment.
I dont disagree with this. But now at least we get to send a custom error to the client. So in that sense it is a little better to catch the error. Especially if there is more than one possible error per request
davemarco
left a comment
There was a problem hiding this comment.
addressed comments
| interface SearchResultsMetadataDocument { | ||
| _id: string; | ||
| errorMsg: Nullable<string>; | ||
| errorMsg: string | null; |
There was a problem hiding this comment.
we dont have nullable type in global common directory yet. I want to change the common directory a bit, but it will require changes to the client outside of this PR. so thats why for now i just did | null
| // eslint-disable-next-line no-warning-comments | ||
| // TODO: update eslint-config-yscope | ||
| "@stylistic/max-len": [ | ||
| "warn", | ||
| { | ||
| code: 100, | ||
| comments: 100, | ||
| ignoreComments: false, | ||
| ignorePattern: "^(import\\s.+\\sfrom\\s.+|\\} from)", | ||
| ignoreRegExpLiterals: true, | ||
| ignoreStrings: false, | ||
| ignoreTemplateLiterals: false, | ||
| ignoreTrailingComments: false, | ||
| ignoreUrls: true, | ||
| tabWidth: 4, | ||
| }, | ||
| ], |
There was a problem hiding this comment.
i needed it before, but maybe after refactoring from #912, it is actually no longer needed. I will remove
| interface SearchResultsMetadataDocument { | ||
| _id: string; | ||
| errorMsg: Nullable<string>; | ||
| errorMsg: string | null; |
There was a problem hiding this comment.
see other comment
| } catch (err: unknown) { | ||
| const errMsg = "Unable to submit search/aggregation job to the SQL database"; | ||
| request.log.error(err, errMsg); | ||
|
|
||
| return reply.internalServerError(errMsg); | ||
| } |
There was a problem hiding this comment.
I dont disagree with this. But now at least we get to send a custom error to the client. So in that sense it is a little better to catch the error. Especially if there is more than one possible error per request
| await mongoDb.collection(searchJobId.toString()).drop(); | ||
| await mongoDb.collection(aggregationJobId.toString()).drop(); |
There was a problem hiding this comment.
Anyways it should not be possible for the collection not to exist. So maybe its fine anyways
| }); | ||
|
|
||
| /* Schema to identify query job */ | ||
| const QueryJobSchema = Type.Object({ |
There was a problem hiding this comment.
Since there are 2 ids, i think QueryJobId is a bit confusing, and prefer to leave it. lmk
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (9)
components/log-viewer-webui/common/searchResultsMetadata.ts (2)
24-24: Consider using theNullabletype alias for consistency.Previous reviewers have suggested using
Nullable<string>instead of the explicit union type for consistency with the codebase conventions.
30-30: Use single-line export format.Previous reviewers have suggested using a single-line export format for consistency.
components/log-viewer-webui/server/src/fastify-v2/schemas/search.ts (1)
23-26: Consider renaming toQueryJobIdSchemafor clarity.Previous reviewers suggested renaming this schema to
QueryJobIdSchemasince it specifically contains job IDs rather than full job information.components/log-viewer-webui/server/src/fastify-v2/routes/api/search/index.ts (6)
1-22: Fix import formatting and ordering issues.The imports have multiple linting issues that need to be addressed:
- Import sorting is incorrect
- Line 17 needs multi-line formatting for consistency
- Missing empty line after imports
Apply this diff to fix the issues:
-import { - SEARCH_SIGNAL, - SearchResultsMetadataDocument, -} from "@common/searchResultsMetadata.js"; -import { - FastifyPluginAsyncTypebox, - Type, -} from "@fastify/type-provider-typebox"; +import {FastifyPluginAsyncTypebox, Type} from "@fastify/type-provider-typebox"; import {StatusCodes} from "http-status-codes"; -import settings from "../../../../../settings.json" with {type: "json"}; +import { + SEARCH_SIGNAL, + SearchResultsMetadataDocument, +} from "@common/searchResultsMetadata.js"; import {ErrorSchema} from "../../../schemas/error.js"; import { QueryJobCreationSchema, QueryJobSchema, } from "../../../schemas/search.js"; -import {SEARCH_MAX_NUM_RESULTS} from "./typings.js"; +import { + SEARCH_MAX_NUM_RESULTS, + SearchResultsMetadataDocument, +} from "./typings.js"; import { createMongoIndexes, updateSearchResultsMeta, updateSearchSignalWhenJobsFinish, } from "./utils.js"; + +import settings from "../../../../../settings.json" with {type: "json"};
36-36: Add missing semicolon.- const mongoDb = mongo.db + const mongoDb = mongo.db;
96-97: Add error handling for MongoDB collection creation.While MongoDB's
createCollectionis idempotent (won't error if collection exists), it's good practice to handle potential errors explicitly for robustness.- await mongoDb.createCollection(searchJobId.toString()); - await mongoDb.createCollection(aggregationJobId.toString()); + try { + await mongoDb.createCollection(searchJobId.toString()); + await mongoDb.createCollection(aggregationJobId.toString()); + } catch (err: unknown) { + // Collection might already exist, which is fine + request.log.debug(err, "Collection creation failed (might already exist)"); + }
123-124: Remove trailing comma.searchJobId: searchJobId, logger: request.log, mongoDb: mongoDb, - });
154-155: Add error handling for collection drop operations.The drop operations should handle cases where collections might not exist (e.g., if
/resultsis called multiple times with the same job IDs).- await mongoDb.collection(searchJobId.toString()).drop(); - await mongoDb.collection(aggregationJobId.toString()).drop(); + try { + await mongoDb.collection(searchJobId.toString()).drop(); + await mongoDb.collection(aggregationJobId.toString()).drop(); + } catch (err: any) { + // Only log/handle if it's not a "collection doesn't exist" error + if (err.code !== 26) { // 26 is MongoDB's "NamespaceNotFound" error code + request.log.error(err, "Unexpected error dropping collections"); + return reply.internalServerError("Failed to drop collections"); + } + // Otherwise, silently ignore - collections were already deleted + }
207-208: Use template literal instead of string concatenation.For consistency and better readability, use a single template literal.
- return reply.internalServerError(`${errMsg} for searchJobId=${searchJobId}, ` + - `aggregationJobId=${aggregationJobId}`); + return reply.internalServerError(`${errMsg} for searchJobId=${searchJobId}, aggregationJobId=${aggregationJobId}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
components/log-viewer-webui/common/searchResultsMetadata.ts(1 hunks)components/log-viewer-webui/server/eslint.config.mjs(1 hunks)components/log-viewer-webui/server/src/fastify-v2/app.ts(2 hunks)components/log-viewer-webui/server/src/fastify-v2/routes/api/search/index.ts(1 hunks)components/log-viewer-webui/server/src/fastify-v2/schemas/search.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/common/searchResultsMetadata.tscomponents/log-viewer-webui/server/src/fastify-v2/app.tscomponents/log-viewer-webui/server/src/fastify-v2/schemas/search.tscomponents/log-viewer-webui/server/src/fastify-v2/routes/api/search/index.ts
🧬 Code Graph Analysis (1)
components/log-viewer-webui/server/src/fastify-v2/schemas/search.ts (1)
components/log-viewer-webui/server/src/fastify-v2/schemas/common.ts (2)
StringSchema(11-11)IdSchema(11-11)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (2)
components/log-viewer-webui/server/src/fastify-v2/app.ts (1)
10-10: LGTM! Good use of standard HTTP status codes.Using
StatusCodesfrom thehttp-status-codespackage instead of magic numbers improves code maintainability and readability.components/log-viewer-webui/server/eslint.config.mjs (1)
24-34: LGTM!The addition of
Type.Nullto thecapIsNewExceptionsarray is necessary to support the TypeBox null type schema used in the new search API routes (e.g., for 204 No Content responses).
| if ("undefined" !== typeof err.statusCode && | ||
| Number(StatusCodes.INTERNAL_SERVER_ERROR) > err.statusCode | ||
| ) { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Improve readability of the typeof check and verify Number() wrapper necessity.
While the logic is correct, the typeof check could be more readable, and the Number() wrapper might be redundant since StatusCodes.INTERNAL_SERVER_ERROR should already be a number.
Consider this more readable approach:
- if ("undefined" !== typeof err.statusCode &&
- Number(StatusCodes.INTERNAL_SERVER_ERROR) > err.statusCode
+ if (typeof err.statusCode !== "undefined" &&
+ err.statusCode < StatusCodes.INTERNAL_SERVER_ERRORLet me verify if the Number() wrapper is actually necessary:
🏁 Script executed:
#!/bin/bash
# Description: Check the type of StatusCodes.INTERNAL_SERVER_ERROR to determine if Number() wrapper is needed
# Search for StatusCodes usage and type definitions
rg -A 3 -B 3 "StatusCodes\.INTERNAL_SERVER_ERROR"
# Look for the http-status-codes package type definitions
fd "http-status-codes" --type fLength of output: 3094
Refactor error status code comparison
The typeof check can be made more idiomatic and the Number() wrapper around StatusCodes.INTERNAL_SERVER_ERROR is unnecessary (it’s already a number). Please update the check in components/log-viewer-webui/server/src/fastify-v2/app.ts (lines 75–77) accordingly:
- if ("undefined" !== typeof err.statusCode &&
- Number(StatusCodes.INTERNAL_SERVER_ERROR) > err.statusCode
+ if (typeof err.statusCode !== "undefined" &&
+ err.statusCode < StatusCodes.INTERNAL_SERVER_ERRORThis change improves readability and removes the redundant conversion.
📝 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 ("undefined" !== typeof err.statusCode && | |
| Number(StatusCodes.INTERNAL_SERVER_ERROR) > err.statusCode | |
| ) { | |
| if (typeof err.statusCode !== "undefined" && | |
| err.statusCode < StatusCodes.INTERNAL_SERVER_ERROR | |
| ) { |
🤖 Prompt for AI Agents
In components/log-viewer-webui/server/src/fastify-v2/app.ts around lines 75 to
77, refactor the typeof check on err.statusCode to a more idiomatic form by
comparing it directly to the string "undefined" using strict equality, and
remove the Number() wrapper around StatusCodes.INTERNAL_SERVER_ERROR since it is
already a number. This will improve readability and eliminate unnecessary type
conversion.
| if ("undefined" === typeof mongoDb) { | ||
| throw new Error("MongoDB database not found"); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use direct undefined comparison.
Prefer direct comparison over string comparison for better readability and consistency with the coding guidelines.
- if ("undefined" === typeof mongoDb) {
+ if (undefined === mongoDb) {
throw new Error("MongoDB database not found");
}📝 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 ("undefined" === typeof mongoDb) { | |
| throw new Error("MongoDB database not found"); | |
| } | |
| if (undefined === mongoDb) { | |
| throw new Error("MongoDB database not found"); | |
| } |
🤖 Prompt for AI Agents
In components/log-viewer-webui/server/src/fastify-v2/routes/api/search/index.ts
at lines 38 to 40, replace the string comparison "undefined" === typeof mongoDb
with a direct undefined comparison by checking if mongoDb === undefined. This
improves readability and aligns with coding guidelines.
|
|
||
| export type {SearchResultsMetadataDocument}; | ||
| export {SEARCH_SIGNAL}; | ||
| export { SEARCH_SIGNAL }; |
There was a problem hiding this comment.
| export { SEARCH_SIGNAL }; | |
| export {SEARCH_SIGNAL}; |
| interface SearchResultsMetadataDocument { | ||
| _id: string; | ||
| errorMsg: Nullable<string>; | ||
| errorMsg: string | null; |
There was a problem hiding this comment.
how about
| errorMsg: string | null; | |
| // eslint-disable-next-line no-warning-comments | |
| // TODO: Replace with Nullable<string> when the `@common` directory refactoring is completed. | |
| errorMsg: string | null; |
| searchJobId: searchJobId, | ||
| logger: request.log, | ||
| mongoDb: mongoDb, | ||
|
|
There was a problem hiding this comment.
seems this suggestion has not been applied
|
For the PR title, how about: |
Description
This PR adds three fastify routes to replace meteor methods from old webui.
The methods were defined here previously:
clp/components/webui/imports/api/search/server/methods.jsThe logic is basically identical. Main changes are related to typescript and porting to fastify.
Minor logic change was required to create searchJob and AggregationJob collections in query route, as
before they were created in meteor publication, which no longer exists.
Checklist
breaking change.
Validation performed
Created a simple test app, which was used to test all three routes. Routes functioned as expected. Here is code for test app. The package must be running to test
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Chores