feat(new-webui): Port search services from old-webui to Fastify plugins. #912
Conversation
WalkthroughThis update introduces new modules for managing query jobs and MongoDB collections related to search results in the log viewer web UI backend. It adds and refactors TypeScript typings for query jobs, updates configuration and TypeScript path aliases, and adjusts import statements for improved modularity and clarity. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FastifyServer
participant QueryJobsDbManager
participant MySQL
participant SearchJobCollectionsManager
participant MongoDB
participant SearchResultsMetadataCollection
Client->>FastifyServer: Submit search/aggregation job
FastifyServer->>QueryJobsDbManager: submitSearchJob(config)
QueryJobsDbManager->>MySQL: INSERT job (with config)
MySQL-->>QueryJobsDbManager: jobId
QueryJobsDbManager-->>FastifyServer: jobId
Client->>FastifyServer: Await job completion
FastifyServer->>QueryJobsDbManager: awaitJobCompletion(jobId)
loop Poll status
QueryJobsDbManager->>MySQL: SELECT status WHERE id=jobId
MySQL-->>QueryJobsDbManager: status
end
QueryJobsDbManager-->>FastifyServer: Job complete
FastifyServer->>SearchJobCollectionsManager: getOrCreateCollection(jobId)
SearchJobCollectionsManager->>MongoDB: Get/Create collection
MongoDB-->>SearchJobCollectionsManager: Collection
SearchJobCollectionsManager-->>FastifyServer: Collection
FastifyServer->>SearchResultsMetadataCollection: Update/read metadata
SearchResultsMetadataCollection->>MongoDB: CRUD on metadata collection
MongoDB-->>SearchResultsMetadataCollection: Result
Suggested reviewers
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure ✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 7
🔭 Outside diff range comments (1)
components/log-viewer-webui/server/src/plugins/DbManager.ts (1)
31-31:⚠️ Potential issueFix the polling interval magic number
TheJOB_COMPLETION_STATUS_POLL_INTERVAL_MILLISis set to0.5ms, which is likely unintentional (half a millisecond). Polling this rapidly could overload the database. Consider using a more reasonable interval (e.g., 500 ms) or reusing a shared constant.Proposed diff:
- const JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS = 0.5; + // Poll every 500 ms instead of 0.5 ms + const JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS = 500;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
components/log-viewer-webui/server/settings.json(1 hunks)components/log-viewer-webui/server/src/fastify-v2/app.ts(2 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/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/routes/query.tscomponents/log-viewer-webui/server/src/plugins/DbManager.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/plugins/app/search/SearchResultsMetadataCollection/index.tscomponents/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchResultsMetadataCollection/typings.tscomponents/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchJobCollectionsManager/typings.tscomponents/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchJobCollectionsManager/index.tscomponents/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.tscomponents/log-viewer-webui/server/src/typings/query.ts
🧠 Learnings (1)
components/log-viewer-webui/server/src/routes/query.ts (1)
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.
🧬 Code Graph Analysis (3)
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/SearchJobCollectionsManager/index.ts (1)
FastifyInstance(86-88)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/SearchResultsMetadataCollection/typings.ts (1)
SearchResultsMetadataDocument(32-32)
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchJobCollectionsManager/index.ts (5)
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchJobCollectionsManager/typings.ts (2)
SearchResultsDocument(24-24)CollectionDroppedError(24-24)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/QueryJobsDbManager/index.ts (1)
FastifyInstance(160-162)components/log-viewer-webui/server/src/fastify-v2/plugins/app/sample.ts (1)
FastifyInstance(31-33)components/log-viewer-webui/server/src/plugins/DbManager.ts (1)
jobId(167-204)
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts (3)
components/log-viewer-webui/server/src/typings/query.ts (5)
QUERY_JOBS_TABLE_COLUMN_NAMES(70-70)QUERY_JOB_TYPE(69-69)QUERY_JOB_STATUS(67-67)QueryJob(64-64)QUERY_JOB_STATUS_WAITING_STATES(68-68)components/log-viewer-webui/server/src/plugins/DbManager.ts (2)
QUERY_JOB_TYPE(225-225)jobId(167-204)components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/typings.ts (1)
JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS(4-4)
🪛 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 (macos-latest)
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (27)
components/log-viewer-webui/server/src/routes/query.ts (1)
7-10: Centralize query typings imports
Good refactor to move the query-related enums intotypings/query.js. This reduces coupling withDbManagertypings and makes the route module self-contained. The use of the.jsextension is correct under ESM with"moduleResolution": "node16".components/log-viewer-webui/server/tsconfig.json (1)
21-21: Approve@common/*alias mapping
Mapping@common/*to../common/*aligns shared modules between client and server, improving import clarity and maintainability.components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/typings.ts (1)
1-4: Externalize job polling interval as a constant
Excellent extraction of the polling-interval magic number into a dedicated constant (JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS). This improves configurability and makes unit testing of the job-wait logic easier.components/log-viewer-webui/server/settings.json (1)
11-11: Add new MongoDB collection name for search metadata
Including"MongoDbSearchResultsMetadataCollectionName": "results-metadata"is correct. Ensure the consuming Fastify plugin imports the setting via the new@settingsalias so that overrides (e.g., in different environments) remain consistent.components/log-viewer-webui/server/src/plugins/DbManager.ts (2)
15-18: Segregate DB and Query typings
Good refactor to extract query-job types out ofDbManagerand import only its own options (DbManagerOptions) and stream-file types. This reduces coupling and clarifies module responsibilities.
19-24: Import query-job enums from dedicated module
ImportingQUERY_JOB_TYPE,QUERY_JOB_STATUS, and related constants fromtypings/query.jsconsolidates query-related definitions in one place, enhancing modularity and easing future maintenance.components/log-viewer-webui/server/src/fastify-v2/app.ts (3)
17-17: New regex pattern for excluding utility and typing files looks good.The
IGNORED_FILES_REGEXpattern will correctly ignore files ending withutils.jsortypings.jsduring autoloading, which is important for properly managing the new search service plugins.
51-51: Application plugins autoloading correctly uses the ignore pattern.Applying the ignore pattern to application plugins ensures that utility and typing files aren't loaded as plugins, which would cause errors since they don't export Fastify plugins.
59-61: Routes autoloading configuration updated with the ignore pattern.The changes maintain the functionality while ensuring consistency with the application plugins configuration by applying the same ignore pattern.
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchResultsMetadataCollection/index.ts (4)
1-6: Imports and type import look good.The file correctly imports dependencies including settings with the JSON module assertion, FastifyInstance type, and fastify-plugin. The type import from the typings file follows good practices for separation of concerns.
8-23: Well implemented MongoDB collection creation function.The function correctly checks for the existence of the MongoDB database before attempting to create the collection, and throws a descriptive error if the database is not found. The collection is properly typed with the
SearchResultsMetadataDocumentinterface.
25-29: Fastify interface extension properly typed.The module declaration extends the FastifyInstance interface to include the SearchResultsMetadataCollection property, which follows Fastify best practices for type safety of decorated properties.
31-41: Fastify plugin implementation is correct.The plugin uses fastify-plugin to ensure proper registration, decorates the fastify instance with the collection, and includes a name for better debugging and error reporting.
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchResultsMetadataCollection/typings.ts (4)
1-2: Import for Nullable type is appropriate.Reusing the Nullable type from common typings promotes code reuse and consistency across the codebase.
4-19: Well-structured SEARCH_SIGNAL enum with clear documentation.The enum clearly defines various states and signals in the search request/response lifecycle with descriptive values and good JSDoc comments explaining their purpose.
21-30: SearchResultsMetadataDocument interface is well-defined.The interface properly defines the structure of MongoDB documents for search results metadata with appropriate types for each field. The JSDoc comment explains the optional nature of the numTotalResults field.
32-33: Clean exports for type and enum.The exports are properly structured, making the type and enum available for import by other modules.
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchJobCollectionsManager/typings.ts (3)
1-11: SearchResultsDocument interface is well-structured.The interface clearly defines the document structure for search results with appropriate types for each field. The JSDoc comment provides a clear explanation of its purpose.
13-21: Custom error class implementation is good.The CollectionDroppedError class properly extends the Error class with a descriptive constructor message and custom name property. This allows for specific error handling when collections are unexpectedly dropped.
23-25: Clean exports for both types.The exports are properly structured, making both the interface and error class available for import by other modules.
components/log-viewer-webui/server/src/typings/query.ts (3)
27-34: Well-structured job status enum.The enum definition is clear and matches the external system as mentioned in the comments. It covers all the necessary statuses for a query job's lifecycle.
39-43: Good use of Set for status grouping.Using a Set for tracking waiting states makes membership checks efficient and the code more maintainable. This will work well with the job polling logic.
57-62: Clear interface definition with enum-based property access.The QueryJob interface extends MySQL's RowDataPacket and uses enum values as computed property names, which improves type safety and maintainability.
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts (2)
47-60: Effective use of MessagePack for job configuration serialization.Using MessagePack to encode the search configuration is a good choice for efficient binary serialization. It maintains smaller payload sizes compared to JSON.
110-156: Well-implemented job polling with proper error handling.The job completion polling logic effectively handles different error cases and job statuses. Good use of the
false === QUERY_JOB_STATUS_WAITING_STATES.has(status)pattern as per coding guidelines.components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchJobCollectionsManager/index.ts (2)
19-27: Good use of private class fields.Using private class fields with the
#prefix enhances encapsulation and follows modern JavaScript/TypeScript practices.
36-42: Proper validation in factory method.The static create method correctly validates the MongoDB database existence before creating the manager instance. This prevents runtime errors that might occur if the database isn't available.
...onents/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts
Outdated
Show resolved
Hide resolved
...onents/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts
Show resolved
Hide resolved
...onents/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts
Outdated
Show resolved
Hide resolved
...g-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchJobCollectionsManager/index.ts
Outdated
Show resolved
Hide resolved
...g-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchJobCollectionsManager/index.ts
Outdated
Show resolved
Hide resolved
...g-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchJobCollectionsManager/index.ts
Outdated
Show resolved
Hide resolved
davemarco
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchResultsMetadataCollection/index.ts (1)
4-4: 🧹 Nitpick (assertive)Use the @settings path alias for settings.json import.
Rather than using a deep relative path, consider using the @settings alias that was previously added to the tsconfig.json file.
-import settings from "../../../../../../settings.json" with {type: "json"}; +import settings from "@settings" with {type: "json"};components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts (4)
6-6: 🧹 Nitpick (assertive)Use Node.js protocol prefix for built-in modules.
Node.js recommends using the
node:protocol prefix for built-in modules to clearly distinguish them from third-party modules.-import {setTimeout} from "timers/promises"; +import {setTimeout} from "node:timers/promises";🧰 Tools
🪛 Biome (1.9.4)
[error] 6-6: 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)
8-8: 🧹 Nitpick (assertive)Use the @settings path alias for settings.json import.
Rather than using a deep relative path, consider using the @settings alias that was previously added to the tsconfig.json file.
-import settings from "../../../../../../settings.json" with {type: "json"}; +import settings from "@settings" with {type: "json"};
81-82: 🧹 Nitpick (assertive)Redundant await keyword on return statement.
The
awaitkeyword is unnecessary when directly returning a Promise, as it adds an extra microtask delay without changing the functionality.- return await this.submitSearchJob(searchAggregationConfig); + return this.submitSearchJob(searchAggregationConfig);
140-141: 🧹 Nitpick (assertive)Consider adding null check before accessing status.
Although it's unlikely given the preceding empty array check, adding a specific null/undefined check for the row would make the code more robust against potential schema changes.
- const status = (rows[0] as QueryJob)[QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS]; + const job = rows[0] as QueryJob; + if (null == job || undefined === job) { + throw new Error(`Invalid job data for job ${jobId}`); + } + const status = job[QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
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/SearchResultsMetadataCollection/index.ts(1 hunks)components/log-viewer-webui/server/tsconfig.json(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/plugins/app/search/SearchResultsMetadataCollection/index.tscomponents/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts
🧬 Code Graph Analysis (1)
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/SearchJobCollectionsManager/index.ts (1)
FastifyInstance(86-88)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/SearchResultsMetadataCollection/typings.ts (1)
SearchResultsMetadataDocument(32-32)
🪛 Biome (1.9.4)
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts
[error] 6-6: 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 (macos-latest)
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (4)
components/log-viewer-webui/server/tsconfig.json (1)
21-21: Good trailing comma addition for future extensibility.Adding a trailing comma after the path mapping makes it easier to add new path mappings in the future without causing syntax errors or unnecessary git diff changes.
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchResultsMetadataCollection/index.ts (2)
1-41: Well-structured Fastify plugin implementation.The plugin correctly follows the Fastify pattern by:
- Using fastify-plugin to expose decorations to parent scopes
- Properly extending the FastifyInstance type via module augmentation
- Adding appropriate error handling for MongoDB connection issues
- Using TypeScript for type safety throughout
16-16: 🧹 Nitpick (assertive)Follow the coding guidelines for condition checks.
The coding guidelines specify a preference for
false == <expression>rather than negation. Consider adapting this check to match the project's style.- if ("undefined" === typeof fastify.mongo.db) { + if (undefined === fastify.mongo.db) {Likely an incorrect or invalid review comment.
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts (1)
1-172: Well-implemented QueryJobsDbManager as a Fastify plugin.This implementation:
- Properly manages query jobs in MySQL
- Uses the factory pattern for construction
- Provides clear methods for job submission, cancellation, and monitoring
- Includes appropriate error handling and status checking
- Correctly integrates with Fastify via plugin decoration
The class effectively encapsulates database operations and provides a clean interface for the rest of the application.
🧰 Tools
🪛 Biome (1.9.4)
[error] 6-6: 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)
| * @return MongoDB collection | ||
| * @throws {Error} if the MongoDB database is not found. |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance JSDoc return type documentation.
The return type in the JSDoc comment could be more specific to help developers understand what's being returned.
- * @return MongoDB collection
+ * @return {Collection<SearchResultsMetadataDocument>} MongoDB collection for search results metadata🤖 Prompt for AI Agents
In
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchResultsMetadataCollection/index.ts
around lines 12 to 13, the JSDoc return type is currently generic. Update the
@return tag to specify the exact MongoDB collection type being returned, such as
including the document interface or schema type, to provide clearer and more
precise documentation for developers.
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | ||
| while (true) { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using a condition in the while loop.
The infinite loop with a break statement could be replaced with a conditional while loop for better readability.
- // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
- while (true) {
+ let jobCompleted = false;
+ while (!jobCompleted) {And then replace the break; on line 151 with:
- break;
+ jobCompleted = true;📝 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.
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | |
| while (true) { | |
| - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | |
| - while (true) { | |
| + let jobCompleted = false; | |
| + while (!jobCompleted) { | |
| @@ | |
| - break; | |
| + jobCompleted = true; |
🤖 Prompt for AI Agents
In
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts
around lines 111 to 112, replace the infinite while(true) loop with a while loop
that uses an explicit condition to control the loop execution. Identify the
condition that currently leads to the break statement on line 151 and use that
condition directly in the while loop. Remove the break statement on line 151
after updating the loop condition to improve readability and maintainability.
| // Loads all application plugins. | ||
| fastify.register(fastifyAutoload, { | ||
| dir: path.join(import.meta.dirname, "plugins/app"), | ||
| ignorePattern: IGNORED_FILES_REGEX, |
There was a problem hiding this comment.
If we need to ignore files here, I'd prefer to import the plugins at the top of the file and register them individually. There are only three anyway.
We could also avoid import.meta.dirname this way
There was a problem hiding this comment.
Instead, how about naming all plugin sources with .plugin.ts extension and using matchFilter to load only those plugin sources?
There was a problem hiding this comment.
there are only three anyway.
There will be more plugins when we refactor the old webui.
If we need to ignore files here, I
We dont explicitly need the filter. It will only load the index files in a directory. I thought maybe if someone adds a shared typings or utils without an index file. It was safer to add this.
m okay with junhao suggestion or just leaving it
There was a problem hiding this comment.
For now i will just remove it. I dont think it will change anything, if someone adds something which breaks it, they can fix then
| dir: path.join(import.meta.dirname, "routes"), | ||
| autoHooks: true, | ||
| cascadeHooks: true, | ||
| dir: path.join(import.meta.dirname, "routes"), |
There was a problem hiding this comment.
This also creates nice routes like api/search/query. Like it will add the folder structure to the route, so i like it
| } 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}`); |
There was a problem hiding this comment.
| } 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}`); | |
| } catch (e: unknown) { | |
| console.error("Failed to query status for job ${jobId}") | |
| throw e; | |
| } |
Current solution removes error's stack trace, which makes the error hard to debug. I prefer two solutions:
- simply
console.error - create a
class JobQueryError { constructor(message, jobId, rawError)}
e 's info is not discarded in both way
There was a problem hiding this comment.
I agree we do not want to discard the stack trace in the original Error. how about
throw new Error("Failed to query status for job ${jobId}", { cause: e });
| if ("undefined" === typeof collection || null === collection) { | ||
| throw new Error(`Collection ${name} not found`); | ||
| } |
There was a problem hiding this comment.
| if ("undefined" === typeof collection || null === collection) { | |
| throw new Error(`Collection ${name} not found`); | |
| } | |
| if ("undefined" === typeof collection) { | |
| throw new Error(`Collection ${name} not found`); | |
| } else if (null === collection) { | |
| throw new CollectionDroppedError(...); | |
| } |
There was a problem hiding this comment.
sure i guess this is okay, but i dont believe this error will be caught anywhere. I can make the change though
| const name = jobId.toString(); | ||
| const collection = this.#jobIdToCollectionsMap.get(name); | ||
| if ("undefined" === typeof collection || null === collection) { | ||
| throw new Error(`Collection ${name} not found`); |
There was a problem hiding this comment.
Consider using CollectionNotFoundError
There was a problem hiding this comment.
we are only using the custom error earlier, since it is caught later in the route see #913 . I dont want to add a custom error if not explicity used.
| if (false === this.#jobIdToCollectionsMap.has(name)) { | ||
| this.#jobIdToCollectionsMap.set(name, this.#db.collection(name)); | ||
| } else if (null === this.#jobIdToCollectionsMap.get(name)) { | ||
| throw new CollectionDroppedError(name); |
There was a problem hiding this comment.
| if (false === this.#jobIdToCollectionsMap.has(name)) { | |
| this.#jobIdToCollectionsMap.set(name, this.#db.collection(name)); | |
| } else if (null === this.#jobIdToCollectionsMap.get(name)) { | |
| throw new CollectionDroppedError(name); | |
| const collection = this.#jobIdToCollectionsMap.get(name) | |
| if ("undefined" === typeof collection) { | |
| this.#jobIdToCollectionsMap.set(name, this.#db.collection(name)); | |
| } else if (null === collection) { | |
| throw new CollectionDroppedError(name); |
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
mostly good. thanks for putting up the changes
| ` | ||
| SELECT ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} | ||
| FROM ${settings.SqlDbQueryJobsTableName} | ||
| WHERE ${QUERY_JOBS_TABLE_COLUMN_NAMES.ID} = ? | ||
| `, |
There was a problem hiding this comment.
| ` | |
| SELECT ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} | |
| FROM ${settings.SqlDbQueryJobsTableName} | |
| WHERE ${QUERY_JOBS_TABLE_COLUMN_NAMES.ID} = ? | |
| `, | |
| ` | |
| SELECT ${QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS} | |
| FROM ${settings.SqlDbQueryJobsTableName} | |
| WHERE ${QUERY_JOBS_TABLE_COLUMN_NAMES.ID} = ? | |
| `, |
| `INSERT INTO ${settings.SqlDbQueryJobsTableName} | ||
| (${QUERY_JOBS_TABLE_COLUMN_NAMES.JOB_CONFIG}, | ||
| ${QUERY_JOBS_TABLE_COLUMN_NAMES.TYPE}) | ||
| VALUES (?, ?)`, |
There was a problem hiding this comment.
| `INSERT INTO ${settings.SqlDbQueryJobsTableName} | |
| (${QUERY_JOBS_TABLE_COLUMN_NAMES.JOB_CONFIG}, | |
| ${QUERY_JOBS_TABLE_COLUMN_NAMES.TYPE}) | |
| VALUES (?, ?)`, | |
| ` | |
| INSERT INTO ${settings.SqlDbQueryJobsTableName} ( | |
| ${QUERY_JOBS_TABLE_COLUMN_NAMES.JOB_CONFIG}, | |
| ${QUERY_JOBS_TABLE_COLUMN_NAMES.TYPE} | |
| ) | |
| VALUES (?, ?) | |
| `, |
...onents/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts
Show resolved
Hide resolved
| `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})`, |
There was a problem hiding this comment.
| `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})`, | |
| ` | |
| 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}) | |
| `, |
| * @return MongoDB collection | ||
| * @throws {CollectionDroppedError} if the collection was already dropped. | ||
| */ | ||
| async getOrCreateCollection (jobId: number): Promise<Collection<SearchResultsDocument>> { |
There was a problem hiding this comment.
see #913, you will see where both methods are used
| * Class to keep track of MongoDB collections created for search jobs, ensuring all collections | ||
| * have unique names. | ||
| */ | ||
| class SearchJobCollectionsManager { |
There was a problem hiding this comment.
it seems the purpose of this plugin is really just to getOrCreateCollection then drop the collection. can we generalize this into a util function / a method in the DbManager?
There was a problem hiding this comment.
I am planning to get rid of db manager, so i dont want to move it there. This is kind of like the dbManager for the old-webui.
This code was in the old webui, so i just tried to keep it the same. Note it wasnt immediately clear to me what is the difference between this and accessing the mongo database methods directly in the route. It appears the only difference is that this getOrCreateCollection() will throw an error if the collection was already dropped. Note we actually catch the custom error in one of the route methods updateSearchSignalWhenJobsFinish(see #913).it may be possible to get rid of this entirely and just use mongo methods directly, but i just tried to keep everything the same to avoid introducing new bugs.
Anyways let me know if we should keep it, or i should attempt to use Mongo methods in the routes.
There was a problem hiding this comment.
sure. let's get rid of this
| } 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}`); |
There was a problem hiding this comment.
I agree we do not want to discard the stack trace in the original Error. how about
throw new Error("Failed to query status for job ${jobId}", { cause: e });
| 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]; |
There was a problem hiding this comment.
how about:
let queryJob: QueryJob | undefined;
try {
...
[queryJob] = queryRows;
} catch () {...}
if ("undefined" === typeof queryJob) {
throw new Error(`Job ${jobId} not found in database.`);
}
const status = queryJob[QUERY_JOBS_TABLE_COLUMN_NAMES.STATUS];
...
There was a problem hiding this comment.
ill try something like this
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts (2)
6-6: Use thenode:protocol for built-in module imports.
timers/promisesis a Node.js core module. Per current Node.js guidelines, prefix it withnode:for clarity.-import {setTimeout} from "timers/promises"; +import {setTimeout} from "node:timers/promises";🧰 Tools
🪛 Biome (1.9.4)
[error] 6-6: 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)
116-151: Infinitewhile (true)hampers readability.Replacing the sentinel
breakwith an explicit loop condition makes the exit logic clearer and removes an ESLint disable. Consider the earlier suggestion:- // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition - while (true) { + let waiting = true; + while (waiting) { ... - break; + waiting = false;components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchJobCollectionsManager/index.ts (1)
74-85: Specify the return type fordropCollection.The method is
asyncbut omits: Promise<void>, unlikegetOrCreateCollection. Adding it improves type clarity and consistency.- async dropCollection (jobId: number) { + async dropCollection (jobId: number): Promise<void> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
components/log-viewer-webui/server/src/fastify-v2/app.ts(1 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/SearchJobCollectionsManager/index.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/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.tscomponents/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchJobCollectionsManager/index.ts
🧬 Code Graph Analysis (2)
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts (3)
components/log-viewer-webui/server/src/typings/query.ts (5)
QUERY_JOBS_TABLE_COLUMN_NAMES(70-70)QUERY_JOB_TYPE(69-69)QUERY_JOB_STATUS(67-67)QueryJob(64-64)QUERY_JOB_STATUS_WAITING_STATES(68-68)components/log-viewer-webui/server/src/plugins/DbManager.ts (2)
QUERY_JOB_TYPE(225-225)jobId(167-204)components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/typings.ts (1)
JOB_COMPLETION_STATUS_POLL_INTERVAL_MILLIS(4-4)
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchJobCollectionsManager/index.ts (4)
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchJobCollectionsManager/typings.ts (2)
SearchResultsDocument(24-24)CollectionDroppedError(24-24)components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts (1)
FastifyInstance(156-158)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/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] 6-6: 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 (1)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (1)
components/log-viewer-webui/server/src/fastify-v2/app.ts (1)
55-58: No functional change detected – safe to skip.The property-ordering change has no behavioural impact, so no action required.
...onents/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts
Outdated
Show resolved
Hide resolved
...g-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchJobCollectionsManager/index.ts
Outdated
Show resolved
Hide resolved
...g-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchJobCollectionsManager/index.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts (1)
117-153:while (true)readability – see earlier reviewThe infinite loop guarded by an internal
breakwas already discussed in a previous review.
Replacing it with an explicit condition (while (QUERY_JOB_STATUS_WAITING_STATES.has(status))) would make the exit criteria obvious and remove the linter suppression comment.components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchJobCollectionsManager/index.ts (1)
74-85: Add an explicit return type todropCollectionThis async method currently relies on TypeScript’s return-type inference.
Declaring it asPromise<void>(like the other public methods) improves readability and consistency, and helps editors surface the correct type information at call sites.- async dropCollection (jobId: number) { + async dropCollection (jobId: number): Promise<void> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
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/SearchJobCollectionsManager/index.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/plugins/app/search/QueryJobsDbManager/index.tscomponents/log-viewer-webui/server/src/fastify-v2/plugins/app/search/SearchJobCollectionsManager/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
| 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.
🧹 Nitpick (assertive)
Prefer array wrapper for parameter binding
mysql2 (and the underlying Fastify MySQL plugin) expect the second argument to be an array (or object) of parameters.
Passing a naked scalar works, but is an undocumented convenience; should the driver change its typing in the future this could break compilation or – worse – mis-bind parameters.
- `,
- jobId,
+ `,
+ [jobId],Sticking to the documented array form keeps the call site consistent with the rest of the codebase and avoids surprises.
📝 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.
| 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, | |
| ); | |
| 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], | |
| ); |
🤖 Prompt for AI Agents
In
components/log-viewer-webui/server/src/fastify-v2/plugins/app/search/QueryJobsDbManager/index.ts
around lines 96 to 105, the query parameter jobId is passed as a scalar instead
of an array. To fix this, wrap jobId in an array when passing it as the second
argument to the query method, ensuring consistent and documented parameter
binding with mysql2 and Fastify MySQL plugin.
|
@junhaoliao I made changes discussed in meeting |
3 participants
Description
The routes in old webui used classes to interact with the databases. I am dubbing these classes "services", and ported them to fastify as plugins. These plugins are required before the routes can be added.
The logic is pretty much identical to old-webui. I made changes to convert to typescript, and to migrate to fastify. I also moved some shared types out of dbManager(dbManager will eventually be removed when old log-viewer-webui code is ported to fastifyv2 - it felt odd to import from a file that I know will be removed later).
PR #913 shows how routes interact with services
The following are main files ported from old webui:
clp/components/webui/imports/api/search/server/QueryJobsDbManager.js/home/davidmawk/clp/components/webui/imports/api/search/SearchJobCollectionsManager.jsclp/components/webui/imports/api/search/server/collections.jsChecklist
breaking change.
Validation performed
Tested that server still runs. Tested functionality in WIP pr, but not this PR specifically.
Summary by CodeRabbit
New Features
Refactor
Chores