feat(new-webui): Add Fastify plugin for real-time MongoDB query service with Socket.IO.#880
Conversation
…separate folder and rename as MongoReplicaServer; fixed the async create() not being `await`ed.
… improved type safety
There was a problem hiding this comment.
Actionable comments posted: 16
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
components/log-viewer-webui/server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
components/log-viewer-webui/common/index.ts(1 hunks)components/log-viewer-webui/server/package.json(2 hunks)components/log-viewer-webui/server/src/app.ts(2 hunks)components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/MongoWatcherCollection.ts(1 hunks)components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts(1 hunks)components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/typings.ts(1 hunks)components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/utils.ts(1 hunks)components/log-viewer-webui/server/tsconfig.json(2 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/app.tscomponents/log-viewer-webui/server/src/plugins/MongoSocketIoServer/utils.tscomponents/log-viewer-webui/server/src/plugins/MongoSocketIoServer/typings.tscomponents/log-viewer-webui/server/src/plugins/MongoSocketIoServer/MongoWatcherCollection.tscomponents/log-viewer-webui/common/index.tscomponents/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts
🧬 Code Graph Analysis (1)
components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/typings.ts (1)
components/log-viewer-webui/common/index.ts (4)
ClientToServerEvents(69-69)ServerToClientEvents(73-73)InterServerEvents(71-71)SocketData(74-74)
🪛 Biome (1.9.4)
components/log-viewer-webui/common/index.ts
[error] 57-59: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts
[error] 133-134: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 328-329: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 333-334: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (9)
components/log-viewer-webui/server/package.json (2)
12-12: Update to the start script path looks good.The updated nodemon watch target path from
dist/src/main.jstodist/server/src/main.jsaligns with the new directory structure.
33-33: Socket.io dependency addition is appropriate.Adding socket.io v4.8.1 is necessary to support the new MongoDB real-time subscription functionality.
components/log-viewer-webui/server/tsconfig.json (2)
18-22: TypeScript path configuration looks good.The addition of
baseUrlandpathsmapping creates a clean alias for importing common types between server and client.
42-43: Including common directory in TypeScript compilation is appropriate.Adding
../common/**/*to the include array ensures shared types are properly processed by the TypeScript compiler.components/log-viewer-webui/server/src/app.ts (2)
11-11: Import for MongoSocketIoServer looks good.The import aligns with the new functionality being added.
64-68: MongoSocketIoServer plugin registration is appropriately conditioned.The plugin is properly registered with the necessary MongoDB connection parameters and follows the same conditional pattern as other plugins.
components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/utils.ts (3)
24-35: MongoDB change stream query transformation looks good.The function correctly transforms standard MongoDB queries into change stream compatible format by prepending "fullDocument." to keys, as required by MongoDB's change event structure.
89-97: Array manipulation utility follows coding guidelines.The
removeItemFromArrayutility correctly follows the project's coding guideline preference forfalse == <expression>rather than!<expression>with the-1 !== indexcheck.
69-81:Details
✅ Verification successful
MongoDB client initialization looks good.
The function properly initializes a MongoDB client and handles connection errors. If authentication is required, it would likely need to be added to the options interface and implementation.
🏁 Script executed:
#!/bin/bash # Check the DbOptions interface to see if it includes authentication parameters fd -e ts -e js "DbOptions" --exec grep -A 10 "interface DbOptions" {} \;Length of output: 75
Let’s locate where
DbOptionsis defined to confirm whether it includes authentication fields:
🏁 Script executed:
#!/bin/bash # Recursively search for DbOptions definitions and usages across TS/JS files rg -n "DbOptions" --glob "*.ts" --glob "*.js"Length of output: 887
Let’s inspect the
DbOptionsinterface to see if it includes any auth fields:
🏁 Script executed:
#!/bin/bash # Show the DbOptions interface definition with a few lines of context rg -n "interface DbOptions" -A 20 components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/typings.tsLength of output: 667
MongoDB client initialization is correct
Confirmed that
DbOptions(incomponents/log-viewer-webui/server/src/plugins/MongoSocketIoServer/typings.ts) currently defines only:
database: stringhost: stringport: numberNo authentication fields are present. The existing
initializeMongoClientimplementation correctly builds the URI and handles connection errors for unauthenticated connections.If your deployment requires authentication, please extend
DbOptions(for example, addusername,password, and/orauthSource) and incorporate those credentials into the MongoDB URI orMongoClientoptions ininitializeMongoClient.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/utils.ts (1)
46-47: Hash still order-dependent for nested objects
Sorting the top-level keys is an improvement, but two semantically identical queries with different nested key orderings will still generate different hashes. If collisions matter, a canonical JSON library (e.g.fast-json-stable-stringify) is safer.components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/typings.ts (1)
42-46:optionsshould be optional to match Mongo’s API
collection.find(filter, options?)acceptsundefined, but the interface forces callers to send{}. Making the property optional prevents unnecessary boiler-plate on the client.- options: FindOptions; + options?: FindOptions;components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/MongoWatcherCollection.ts (2)
26-27: Private field casing inconsistent with project style
#queryIdtoWatcherMapshould be#queryIdToWatcherMap(camel-case after acronym) for consistency with other identifiers.
70-73: Typo:unsubcribe→unsubscribe
Misspelled method propagates to every call-site and slows discovery via IDE auto-completion. Please rename and refactor usages.components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts (1)
243-247: 🧹 Nitpick (assertive)Coding-guideline violation: avoid
!collectionName
Project guidelines prefer explicit comparison (false == expr). Replace with:-if (!collectionName) { +if ("undefined" === typeof collectionName) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/MongoWatcherCollection.ts(1 hunks)components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts(1 hunks)components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/typings.ts(1 hunks)components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/utils.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/plugins/MongoSocketIoServer/utils.tscomponents/log-viewer-webui/server/src/plugins/MongoSocketIoServer/MongoWatcherCollection.tscomponents/log-viewer-webui/server/src/plugins/MongoSocketIoServer/typings.tscomponents/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts
🧠 Learnings (1)
components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/MongoWatcherCollection.ts (2)
Learnt from: davemarco
PR: y-scope/clp#880
File: components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/MongoWatcherCollection.ts:79-83
Timestamp: 2025-05-05T18:03:50.461Z
Learning: In MongoWatcherCollection, the unsubscribe method is intentionally designed to remove only one instance of a connection ID from the subscribers array using removeItemFromArray. This allows for multiple subscriptions from the same connection, with each unsubscribe call removing only one subscription at a time.
Learnt from: davemarco
PR: y-scope/clp#880
File: components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/MongoWatcherCollection.ts:196-200
Timestamp: 2025-05-05T18:08:23.286Z
Learning: In the MongoSocketIoServer plugin, change streams are designed to be closed only through explicit client unsubscribe actions rather than having automatic cleanup on error or unexpected close events. This design choice puts the responsibility on the client to properly unsubscribe.
🪛 Biome (1.9.4)
components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts
[error] 133-134: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 328-329: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 333-334: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
| const initializeMongoClient = async ( | ||
| options: DbOptions | ||
| ): Promise<Db> => { | ||
| const mongoUri = `mongodb://${options.host}:${options.port}`; | ||
| const mongoClient = new MongoClient(mongoUri); | ||
| try { | ||
| await mongoClient.connect(); | ||
|
|
||
| return mongoClient.db(options.database); | ||
| } catch (e) { | ||
| throw new Error("MongoDB connection error", {cause: e as Error}); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
No credentials / options passed to MongoClient
initializeMongoClient hard-codes a basic mongodb://host:port URI. In production you will likely need credentials, SSL options, or SRV records. Consider accepting a full connectionString (or extra options) so the plugin can be reused in secured deployments.
| const convertQueryToChangeStreamFormat = ( | ||
| query: Filter<Document> | ||
| ): Filter<Document> => { | ||
| const changeStreamQuery: Filter<Document> = {}; | ||
| for (const key in query) { | ||
| if (Object.hasOwn(query, key)) { | ||
| changeStreamQuery[`fullDocument.${key}`] = query[key] as unknown; | ||
| } | ||
| } | ||
|
|
||
| return changeStreamQuery; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Possible loss of filter accuracy for nested queries
convertQueryToChangeStreamFormat only rewrites the first-level keys of the filter.
If the client sends {a: {b: 1}} the produced $match becomes { "fullDocument.a": { b: 1 } }, which does not match updates where fullDocument.a.b changes. A recursive walk (or use of Mongo’s dot-notation on sub-paths) is required for parity with the original find query.
-for (const key in query) {
- if (Object.hasOwn(query, key)) {
- changeStreamQuery[`fullDocument.${key}`] = query[key] as unknown;
- }
-}
+const build = (src: Record<string, unknown>, prefix = "") => {
+ for (const [k, v] of Object.entries(src)) {
+ const path = prefix ? `${prefix}.${k}` : k;
+ if (v && "object" === typeof v && false === Array.isArray(v)) {
+ build(v as Record<string, unknown>, path);
+ } else {
+ changeStreamQuery[`fullDocument.${path}`] = v as unknown;
+ }
+ }
+};
+build(query as Record<string, unknown>);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const convertQueryToChangeStreamFormat = ( | |
| query: Filter<Document> | |
| ): Filter<Document> => { | |
| const changeStreamQuery: Filter<Document> = {}; | |
| for (const key in query) { | |
| if (Object.hasOwn(query, key)) { | |
| changeStreamQuery[`fullDocument.${key}`] = query[key] as unknown; | |
| } | |
| } | |
| return changeStreamQuery; | |
| }; | |
| const convertQueryToChangeStreamFormat = ( | |
| query: Filter<Document> | |
| ): Filter<Document> => { | |
| const changeStreamQuery: Filter<Document> = {}; | |
| const build = (src: Record<string, unknown>, prefix = "") => { | |
| for (const [k, v] of Object.entries(src)) { | |
| const path = prefix ? `${prefix}.${k}` : k; | |
| if (v && "object" === typeof v && false === Array.isArray(v)) { | |
| build(v as Record<string, unknown>, path); | |
| } else { | |
| changeStreamQuery[`fullDocument.${path}`] = v as unknown; | |
| } | |
| } | |
| }; | |
| build(query as Record<string, unknown>); | |
| return changeStreamQuery; | |
| }; |
| "Subscribed queryIDs map" + | ||
| ` ${JSON.stringify(Array.from(this.#subscribedQueryIdsMap.entries()))}` | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use template literals instead of string concatenation
- this.#fastify.log.debug(
- "Subscribed queryIDs map" +
- ` ${JSON.stringify(Array.from(this.#subscribedQueryIdsMap.entries()))}`
- );
+ this.#fastify.log.debug(
+ `Subscribed queryIDs map ${JSON.stringify(
+ Array.from(this.#subscribedQueryIdsMap.entries())
+ )}`
+ );📝 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.
| "Subscribed queryIDs map" + | |
| ` ${JSON.stringify(Array.from(this.#subscribedQueryIdsMap.entries()))}` | |
| ); | |
| this.#fastify.log.debug( | |
| `Subscribed queryIDs map ${JSON.stringify( | |
| Array.from(this.#subscribedQueryIdsMap.entries()) | |
| )}` | |
| ); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 133-134: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
| const subscribedQueryIds = this.#subscribedQueryIdsMap.get(socket.id); | ||
|
|
||
| if ("undefined" === typeof subscribedQueryIds) { | ||
| return; | ||
| } | ||
|
|
||
| for (const queryId of subscribedQueryIds) { | ||
| this.#unsubscribe(socket, queryId); | ||
| } |
There was a problem hiding this comment.
Duplicate-subscription leak on disconnect
for (const queryId of subscribedQueryIds) iterates while subscribedQueryIds is being mutated inside #unsubscribe → removeItemFromArray.
When the first occurrence is removed the internal iterator skips the next duplicate, leaving dangling subscriptions (watcher still thinks the socket is subscribed).
-for (const queryId of subscribedQueryIds) {
- this.#unsubscribe(socket, queryId);
-}
+for (const queryId of [...subscribedQueryIds]) { // iterate over a copy
+ this.#unsubscribe(socket, queryId);
+}Alternatively store a Set<QueryId> per socket to sidestep the issue entirely.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const subscribedQueryIds = this.#subscribedQueryIdsMap.get(socket.id); | |
| if ("undefined" === typeof subscribedQueryIds) { | |
| return; | |
| } | |
| for (const queryId of subscribedQueryIds) { | |
| this.#unsubscribe(socket, queryId); | |
| } | |
| const subscribedQueryIds = this.#subscribedQueryIdsMap.get(socket.id); | |
| if ("undefined" === typeof subscribedQueryIds) { | |
| return; | |
| } | |
| - for (const queryId of subscribedQueryIds) { | |
| - this.#unsubscribe(socket, queryId); | |
| - } | |
| + for (const queryId of [...subscribedQueryIds]) { // iterate over a copy | |
| + this.#unsubscribe(socket, queryId); | |
| + } |
There was a problem hiding this comment.
linking #822 here as a future action item (just as a note; we will refactor this with npm workspaces in a separate PR)
| "lint:fix": "npm run lint:check -- --fix", | ||
| "prod": "NODE_ENV=production node dist/src/main.js", | ||
| "start": "NODE_ENV=development concurrently \"npm run build:watch\" \"nodemon dist/src/main.js\"", | ||
| "start": "NODE_ENV=development concurrently \"npm run build:watch\" \"nodemon dist/server/src/main.js\"", |
There was a problem hiding this comment.
The start-clp.sh script does not seem to work with the new directory structure:
...
2025-05-07T13:56:34.733 INFO [start_clp] Starting webui...
2025-05-07T13:56:35.129 INFO [start_clp] Started webui.
2025-05-07T13:56:35.130 INFO [start_clp] Starting log_viewer_webui...
2025-05-07T13:56:35.315 ERROR [start_clp] Failed to start CLP: webui /home/junhao/workspace/clp/build/clp-package/var/www/log-viewer-webui/server/dist/settings.json is not a valid path to settings.json
Maybe we need to update start-clp.py as well. Alternatively, we can keep the directory structure unchanged and duplicate the definitions until #822 is addressed
There was a problem hiding this comment.
I think we need to update the prod command as well:
"prod": "NODE_ENV=production node dist/server/src/main.js",
But not sure this is enough. There may some other hardcode, somewhere. I will try some things out
There was a problem hiding this comment.
Also I found the other hardcode here.
/home/davidmawk/clp/components/clp-package-utils/clp_package_utils/scripts/start_clp.py
settings_json_path = (
get_clp_home() / "var" / "www" / "log-viewer-webui" / "server" / "dist" / "settings.json"
)
There was a problem hiding this comment.
I tested package and it ran
junhaoliao
left a comment
There was a problem hiding this comment.
mostly lgtm. i left some nit picking comments
| if (1 < watcher.subscribers.length) { | ||
| removeItemFromArray(watcher.subscribers, connectionId); |
There was a problem hiding this comment.
would it be better to write
| if (1 < watcher.subscribers.length) { | |
| removeItemFromArray(watcher.subscribers, connectionId); | |
| removeItemFromArray(watcher.subscribers, connectionId); | |
| if (this.isReferenced()) { |
so we can streamline the ref check?
There was a problem hiding this comment.
As above, isReferenced is not related to this function
| * | ||
| * @param queryId | ||
| * @param connectionId | ||
| * @return True if connection is last subcriber, false otherwise. |
There was a problem hiding this comment.
if my understanding is correct, would it be better to write
| * @return True if connection is last subcriber, false otherwise. | |
| * @return True if the collection is no longer referenced, false otherwise. |
There was a problem hiding this comment.
Not exactly, since this is only for a single queryID in the collection, not the entire collection
| requestArgs: {collectionName: string}, | ||
| callback:(res: Response<void>) => void | ||
| ): Promise<void> { | ||
| const {collectionName} = requestArgs; |
There was a problem hiding this comment.
It might look cleaner to destructure directly in the method signature, but i'll let you decide
| requestArgs: {collectionName: string}, | |
| callback:(res: Response<void>) => void | |
| ): Promise<void> { | |
| const {collectionName} = requestArgs; | |
| {collectionName}: {collectionName: string}, | |
| callback:(res: Response<void>) => void | |
| ): Promise<void> { |
There was a problem hiding this comment.
I dont hate it, but i like how it current matches the typings in common.ts
| } | ||
|
|
||
| watcher.changeStream.close().catch((err: unknown) => { | ||
| console.error(`Error closing watcher for queryID:${queryId}:`, err); |
There was a problem hiding this comment.
This is minor but ideally we want to use the Fastify logger for better context and log routing when we write redirect logs to a file. One possible solution is to maintain a logger prop in the class with default being console, then accept an optional logger init parameter in the constructor for overriding if non-null.
the above suggestion is just to avoid this class depending on Fastify. since we're not using this class anywhere else, it's also fine if we directly maintain a Fastify instance in the class.
If you think the benefit is minor compared to getting this merged on time (so others are unblocked), we can create an issue to track this and address in a separate PR.
There was a problem hiding this comment.
I think this is good to pass the logger. Lets create an issue
@coderabbitai can you make an issue for this
There was a problem hiding this comment.
🧩 Analysis chain
@davemarco I've created an issue to track the suggestion of replacing direct console usage with a proper logger in the MongoWatcherCollection class. The issue includes the proposed solution to accept an optional logger parameter in the constructor while maintaining console as the default.
🏁 Script executed:
#!/bin/bash
# Find all console logging statements in the MongoWatcherCollection.ts file
grep -n "console." components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/MongoWatcherCollection.ts
Length of output: 503
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for replacing console logging with a proper logger
gh issue create \
--title "Replace direct console usage with logger in MongoWatcherCollection" \
--body "## Description
Currently, the MongoWatcherCollection class uses direct console.warn and console.error calls for logging. As suggested in [PR #880](https://github.com/y-scope/clp/pull/880#discussion_r2078461425), we should replace these with a proper logger for better context and log routing when logs are written to a file.
## Proposed Solution
Modify the MongoWatcherCollection class to:
1. Accept an optional logger parameter in the constructor
2. Default to using console if no logger is provided
3. Replace all direct console.warn and console.error calls with the logger
This will improve log handling and make the class more flexible while maintaining compatibility with current usage.
## References
- PR: #880
- [Comment thread](https://github.com/y-scope/clp/pull/880#discussion_r2078461425)
" \
--assignee davemarco
Length of output: 972
@davemarco I've created issue #885 "Replace direct console usage with logger in MongoWatcherCollection" and assigned it to you. The issue covers all five console logging instances identified in the file and outlines the proposed approach of accepting an optional logger parameter in the constructor.
🧬 Code Graph Analysis Results
Relevant snippets for understanding MongoWatcherCollection.ts and the user comment about passing the logger:
From components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/utils.ts
-
Line 100:
convertQueryToChangeStreamFormatThis utility function is used in
MongoWatcherCollection.createWatcher()to convert a query into the format required by MongoDB change streams. -
Line 104:
removeItemFromArrayThis utility function is used in
MongoWatcherCollection.unsubcribe()and in other places to remove an item from an array, e.g., removing a subscriber connection ID from the watcher’s subscriber list.
From components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/typings.ts
-
Line 73:
CLIENT_UPDATE_TIMEOUT_MSThis constant is used in
MongoWatcherCollection.#setupWatcherListener()to throttle how often updates are emitted to clients. -
Line 76:
MongoCustomSocketThis type is used as the type for socket parameters in
subscribe()and other methods, representing the socket connection. -
Line 77:
QueryIdUsed as the type for query identifiers in watcher maps and method parameters.
-
Line 78:
QueryParametersRepresents the parameters of a MongoDB query, used in methods like
createWatcher(),find(), and#setupWatcherListener(). -
Line 79:
WatcherRepresents the watcher object containing the MongoDB change stream and subscriber list, used internally in the class.
From components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts
Several snippets here show how MongoWatcherCollection is used and how logging is performed, which is relevant to the user comment about passing the logger:
-
Lines 235-270:
#collectionFindSubscribeListenerThis async method handles a socket request to subscribe to a query on a collection. It creates or retrieves a
MongoWatcherCollectioninstance, subscribes the socket to the query, fetches initial documents, and logs info messages usingthis.#fastify.log. -
Lines 302-337:
#unsubscribeThis method unsubscribes a socket from a query, calls
MongoWatcherCollection.unsubcribe(), and logs info and error messages. It also handles cleanup of query maps and collections if no longer referenced. -
Lines 346-373:
#collectionFindUnsubscribeListenerHandles socket requests to unsubscribe from a query, calls
#unsubscribe, and logs accordingly. -
Lines 119-136:
#disconnectListenerHandles socket disconnection, unsubscribes from all queries, and logs the events.
-
Lines 157-178:
#collectionInitListenerHandles socket requests to initialize a collection, checks if the collection exists, and logs errors or info.
-
Lines 187-197:
#addQueryIdToSubscribedListManages internal subscription tracking for sockets.
-
Lines 205-223:
#getQueryIdGenerates or retrieves a query ID for a given query.
-
Lines 144-147:
#collectionExistsChecks if a MongoDB collection exists.
Summary
The above snippets provide context on:
- How
MongoWatcherCollectionis instantiated and used in the server plugin. - How logging is currently done outside
MongoWatcherCollection(in the server plugin class) usingthis.#fastify.log. - The utility functions and types used inside
MongoWatcherCollection. - The user comment about passing the logger likely refers to improving logging inside
MongoWatcherCollectionby passing a logger instance to it, as currently logging is done viaconsoleinsideMongoWatcherCollection.
These snippets are relevant for creating an issue or making changes related to passing a logger instance into MongoWatcherCollection for better logging practices.
[End of relevant snippets]
🧠 Learnings used
Learnt from: davemarco
PR: y-scope/clp#880
File: components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/MongoWatcherCollection.ts:79-83
Timestamp: 2025-05-05T18:03:50.472Z
Learning: In MongoWatcherCollection, the unsubscribe method is intentionally designed to remove only one instance of a connection ID from the subscribers array using removeItemFromArray. This allows for multiple subscriptions from the same connection, with each unsubscribe call removing only one subscription at a time.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| */ | ||
| async #collectionFindSubscribeListener ( | ||
| socket: MongoCustomSocket, | ||
| requestArgs: {query: object; options: object}, |
There was a problem hiding this comment.
ditto (if we see benefits in destructuring directly in the method signature)
| */ | ||
| async #collectionFindUnsubscribeListener ( | ||
| socket: MongoCustomSocket, | ||
| requestArgs: {queryId: number} |
There was a problem hiding this comment.
ditto (if we see benefits in destructuring directly in the method signature)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (13)
components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/typings.ts (4)
27-29: 🧹 Nitpick (assertive)Typo in documentation string ("repsent" → "represent").
Minor spelling issue in the doc-comment for
ConnectionId./** - * Unique ID to repsent each socket connection. + * Unique ID to represent each socket connection. */
35-39: 🧹 Nitpick (assertive)
optionsshould be optional to match Mongo'sfind()usage.
FindOptionsparameters are optional when callingcollection.find(). Declaring the field as mandatory forces callers to pass{}even when no options are needed and breaks type-safety with existing client code that sends onlyquery.interface QueryParameters { collectionName: string; query: Filter<Document>; - options: FindOptions; + options?: FindOptions; }
54-54: 🧹 Nitpick (assertive)Improve constant name clarity by specifying time unit.
Following the coding convention noted in previous review feedback, include "MILLIS" in the constant name to clearly indicate the unit of measurement and avoid ambiguity with microseconds.
-const CLIENT_UPDATE_TIMEOUT_MILLIS = 500; +const CLIENT_UPDATE_TIMEOUT_MILLIS = 500;
6-7: 🧹 Nitpick (assertive)Consider moving QueryId type to common location.
Per previous review comment, the QueryId type could be moved to common.ts to promote reuse and consistency across modules.
components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/MongoWatcherCollection.ts (4)
25-27: 🧹 Nitpick (assertive)
#queryIdtoWatcherMapviolates camel-case convention.Rename to
#queryIdToWatcherMapfor consistency with other identifiers.- #queryIdtoWatcherMap: Map<QueryId, Watcher> = new Map(); + #queryIdToWatcherMap: Map<QueryId, Watcher> = new Map();
70-70: 🧹 Nitpick (assertive)Method name is misspelled (
unsubcribe).Consistent spelling matters for discoverability and auto-completion. Rename to
unsubscribe()(and update all call-sites) to avoid future confusion.- unsubcribe (queryId: number, connectionId: string): boolean { + unsubscribe (queryId: number, connectionId: string): boolean {
177-177: 🧹 Nitpick (assertive)Guideline violation: use
false == emitTimeoutinstead of!emitTimeout.Project coding guidelines ask for the "yoda" style boolean comparison.
- } else if (null === emitTimeout) { + } else if (null === emitTimeout) {
86-86: 🧹 Nitpick (assertive)Consider replacing direct console usage with logger.
Using direct console.error calls makes it harder to route logs to files and add contextual information. As discussed in previous comments and tracked in issue #885, consider accepting an optional logger parameter in the constructor.
components/log-viewer-webui/common/index.ts (2)
66-67: Empty interface: same feedback as beforeBiome again warns that an empty
interfaceis equivalent to{}. Our previous discussion concluded you were happy to keep it, so no further action unless you have changed your mind.🧰 Tools
🪛 Biome (1.9.4)
[error] 65-67: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
26-26:Response<T>still lacks a discriminantThe union
Err | Success<T>remains hard to narrow without a shared literal field (e.g.,status: "ok" | "error"). This mirrors the point raised in the earlier review; just flagging in case you reconsider.components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts (3)
243-247: Callback never fired on bad state (repeat note)When
collectionNameis missing the function logs and returns, but the client-supplied callback is never invoked, leaving the caller hanging. This has been mentioned earlier; flagging again only for completeness.
127-129: Possible mutation-during-iteration (already discussed)Looping over
subscribedQueryIdswhile calling#unsubscribecould skip elements if that array were mutated inside#unsubscribe. You’ve clarified the current implementation is safe, so this is just a historical note.
328-335: 🧹 Nitpick (assertive)Same concatenation issue here
Switching to a single template literal will silence Biome and improve readability:
- this.#fastify.log.debug( - "Query ID to query hash map:" + - ` ${JSON.stringify(Array.from(this.#queryIdToQueryHashMap.entries()))}` - ); + this.#fastify.log.debug( + `Query ID to query hash map: ${JSON.stringify( + Array.from(this.#queryIdToQueryHashMap.entries()) + )}` + );🧰 Tools
🪛 Biome (1.9.4)
[error] 328-329: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 333-334: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
components/clp-package-utils/clp_package_utils/scripts/start_clp.py(2 hunks)components/log-viewer-webui/common/index.ts(1 hunks)components/log-viewer-webui/server/package.json(2 hunks)components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/MongoWatcherCollection.ts(1 hunks)components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts(1 hunks)components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/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/plugins/MongoSocketIoServer/typings.tscomponents/log-viewer-webui/server/src/plugins/MongoSocketIoServer/MongoWatcherCollection.tscomponents/log-viewer-webui/common/index.tscomponents/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts
🧠 Learnings (1)
components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/MongoWatcherCollection.ts (2)
Learnt from: davemarco
PR: y-scope/clp#880
File: components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/MongoWatcherCollection.ts:79-83
Timestamp: 2025-05-05T18:03:50.472Z
Learning: In MongoWatcherCollection, the unsubscribe method is intentionally designed to remove only one instance of a connection ID from the subscribers array using removeItemFromArray. This allows for multiple subscriptions from the same connection, with each unsubscribe call removing only one subscription at a time.
Learnt from: davemarco
PR: y-scope/clp#880
File: components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/MongoWatcherCollection.ts:196-200
Timestamp: 2025-05-05T18:08:23.313Z
Learning: In the MongoSocketIoServer plugin, change streams are designed to be closed only through explicit client unsubscribe actions rather than having automatic cleanup on error or unexpected close events. This design choice puts the responsibility on the client to properly unsubscribe.
🧬 Code Graph Analysis (2)
components/clp-package-utils/clp_package_utils/scripts/start_clp.py (1)
components/clp-package-utils/clp_package_utils/general.py (1)
get_clp_home(94-110)
components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/typings.ts (1)
components/log-viewer-webui/common/index.ts (4)
ClientToServerEvents(77-77)ServerToClientEvents(81-81)InterServerEvents(79-79)SocketData(82-82)
🪛 Biome (1.9.4)
components/log-viewer-webui/common/index.ts
[error] 65-67: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts
[error] 133-134: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 328-329: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 333-334: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (7)
components/log-viewer-webui/server/package.json (2)
11-12: Build output path changes align with directory structure updates.The script paths for "prod" and "start" have been updated from
dist/src/main.jstodist/server/src/main.js, reflecting the restructured output directory. This change matches the corresponding path updates in start_clp.py.
33-33: Socket.io dependency added to support MongoDB query subscription feature.The addition of socket.io v4.8.1 as a dependency is appropriate for implementing the real-time MongoDB query subscription system described in the PR objectives.
components/clp-package-utils/clp_package_utils/scripts/start_clp.py (2)
942-942: Path updated to match new directory structure.The settings.json path has been updated to include the additional
/server/directory segment, aligning with the directory restructuring in other files.
1030-1030: Node.js main script path updated to match new directory structure.The path to the main.js file has been updated to include the additional
/server/directory segment, maintaining consistency with the changes in package.json scripts.components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/MongoWatcherCollection.ts (2)
196-200: Noerror/closehandling on the change-stream listener.This is intentional per the previous review comments and learnings, where change streams are designed to be closed only through explicit client unsubscribe actions.
I'm using the retrieved learning: "In the MongoSocketIoServer plugin, change streams are designed to be closed only through explicit client unsubscribe actions rather than having automatic cleanup on error or unexpected close events. This design choice puts the responsibility on the client to properly unsubscribe."
79-83: Only one subscriber instance is removed; duplicates may remain.This implementation intentionally removes just one instance of a connection ID from subscribers, allowing for multiple subscriptions from the same connection as confirmed in the retrieved learnings.
I'm using the retrieved learning: "In MongoWatcherCollection, the unsubscribe method is intentionally designed to remove only one instance of a connection ID from the subscribers array using removeItemFromArray. This allows for multiple subscriptions from the same connection, with each unsubscribe call removing only one subscription at a time."
components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts (1)
250-252: 🧹 Nitpick (assertive)Prefer template literals for log statements
String concatenation is harder to read and was already flagged by Biome. A quick refactor keeps the style consistent across the file:
- this.#fastify.log.info( - `Socket:${socket.id} requested query:${JSON.stringify(query)} ` + - `with options:${JSON.stringify(options)} to collection:${collectionName}` - ); + this.#fastify.log.info( + `Socket:${socket.id} requested query:${JSON.stringify(query)} ` + + `with options:${JSON.stringify(options)} to collection:${collectionName}` + );Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/MongoWatcherCollection.ts (1)
26-27: Rename#queryIdtoWatcherMap→#queryIdToWatcherMapfor camel-case consistencySame nit was raised earlier and accepted, but the variable has not been updated yet.
components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts (3)
243-247: Callback is never invoked on early return – client will hang
Same concern was raised previously. Even if you believe this branch is unreachable, defensively invoking the callback with an error keeps the contract consistent and prevents hard-to-debug client stalls.
133-135: 🧹 Nitpick (assertive)Prefer template literals over string concatenation
- this.#fastify.log.debug( - "Subscribed queryIDs map" + - ` ${JSON.stringify(Array.from(this.#subscribedQueryIdsMap.entries()))}` - ); + this.#fastify.log.debug( + `Subscribed queryIDs map ${JSON.stringify( + Array.from(this.#subscribedQueryIdsMap.entries()) + )}` + );🧰 Tools
🪛 Biome (1.9.4)
[error] 133-134: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
328-330: 🧹 Nitpick (assertive)Use template literals for clearer logging
- this.#fastify.log.debug( - "Query ID to query hash map:" + - ` ${JSON.stringify(Array.from(this.#queryIdToQueryHashMap.entries()))}` - ); + this.#fastify.log.debug( + `Query ID to query hash map: ${JSON.stringify( + Array.from(this.#queryIdToQueryHashMap.entries()) + )}` + );🧰 Tools
🪛 Biome (1.9.4)
[error] 328-329: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
components/log-viewer-webui/common/index.ts(1 hunks)components/log-viewer-webui/server/eslint.config.mjs(1 hunks)components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/MongoWatcherCollection.ts(1 hunks)components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/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/common/index.tscomponents/log-viewer-webui/server/src/plugins/MongoSocketIoServer/MongoWatcherCollection.tscomponents/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts
🧠 Learnings (2)
components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/MongoWatcherCollection.ts (2)
Learnt from: davemarco
PR: y-scope/clp#880
File: components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/MongoWatcherCollection.ts:79-83
Timestamp: 2025-05-05T18:03:50.472Z
Learning: In MongoWatcherCollection, the unsubscribe method is intentionally designed to remove only one instance of a connection ID from the subscribers array using removeItemFromArray. This allows for multiple subscriptions from the same connection, with each unsubscribe call removing only one subscription at a time.
Learnt from: davemarco
PR: y-scope/clp#880
File: components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/MongoWatcherCollection.ts:196-200
Timestamp: 2025-05-05T18:08:23.313Z
Learning: In the MongoSocketIoServer plugin, change streams are designed to be closed only through explicit client unsubscribe actions rather than having automatic cleanup on error or unexpected close events. This design choice puts the responsibility on the client to properly unsubscribe.
components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts (1)
Learnt from: davemarco
PR: y-scope/clp#880
File: components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/MongoWatcherCollection.ts:79-83
Timestamp: 2025-05-05T18:03:50.472Z
Learning: In MongoWatcherCollection, the unsubscribe method is intentionally designed to remove only one instance of a connection ID from the subscribers array using removeItemFromArray. This allows for multiple subscriptions from the same connection, with each unsubscribe call removing only one subscription at a time.
🪛 Biome (1.9.4)
components/log-viewer-webui/common/index.ts
[error] 65-67: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
components/log-viewer-webui/server/src/plugins/MongoSocketIoServer/index.ts
[error] 133-134: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 328-329: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 333-334: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
🔇 Additional comments (1)
components/log-viewer-webui/server/eslint.config.mjs (1)
8-13: 👍 Ignore list update looks goodAdding build-output folders (
yscope-log-viewer/dist/,client/dist/) prevents unnecessary linting noise and speeds up CI runs.
| requestArgs: { | ||
| query: object; | ||
| options: object; | ||
| }, | ||
| callback: (res: Response<{queryId: QueryId; initialDocuments: object[]}>) => void) => void; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use Record<string, unknown> instead of object for stronger typing
object allows values like null, which is unlikely a valid MongoDB query/options object and defeats type-safety. Replacing the two object occurrences with Record<string, unknown> keeps the shape open while preventing accidental null/primitive inputs.
- requestArgs: {
- query: object;
- options: object;
+ requestArgs: {
+ query: Record<string, unknown>;
+ options: Record<string, unknown>;📝 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.
| requestArgs: { | |
| query: object; | |
| options: object; | |
| }, | |
| callback: (res: Response<{queryId: QueryId; initialDocuments: object[]}>) => void) => void; | |
| requestArgs: { | |
| query: Record<string, unknown>; | |
| options: Record<string, unknown>; | |
| }, | |
| callback: (res: Response<{queryId: QueryId; initialDocuments: object[]}>) => void) => void; |
| let lastEmitTime = 0; | ||
| let emitTimeout: NodeJS.Timeout | null = null; | ||
|
|
||
| const emitUpdateWithTimeout = async () => { | ||
| const currentTime = Date.now(); | ||
| if (CLIENT_UPDATE_TIMEOUT_MILLIS <= currentTime - lastEmitTime) { | ||
| lastEmitTime = currentTime; | ||
| const data = await this.find(queryParameters); | ||
| emitUpdate(data); | ||
| } else if (null === emitTimeout) { | ||
| const delay = CLIENT_UPDATE_TIMEOUT_MILLIS - (currentTime - lastEmitTime); | ||
|
|
||
| emitTimeout = setTimeout(() => { | ||
| emitTimeout = null; | ||
|
|
||
| const fetchAndEmit = async () => { | ||
| const data = await this.find(queryParameters); | ||
| emitUpdate(data); | ||
| }; | ||
|
|
||
| fetchAndEmit().catch((error: unknown) => { | ||
| console.error("Error in emitUpdatesWithTimeout:", error); | ||
| }); | ||
| lastEmitTime = Date.now(); | ||
| }, delay); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clear pending timeout when watcher is closed to avoid leaked timers
If the client unsubscribes shortly after a change event, emitTimeout may still fire even though the change-stream has been closed, needlessly executing a find() and holding references.
- watcher.changeStream.on("change", () => {
+ const onChange = () => {
emitUpdateWithTimeout().catch((error: unknown) => {
console.error("Error in emitUpdatesWithTimeout:", error);
});
- });
+ };
+ watcher.changeStream.on("change", onChange);
+
+ // Cleanup when the change-stream is closed.
+ watcher.changeStream.on("close", () => {
+ if (null !== emitTimeout) {
+ clearTimeout(emitTimeout);
+ emitTimeout = null;
+ }
+ watcher.changeStream.off("change", onChange);
+ });There was a problem hiding this comment.
adding @haiqi96 here since the package scripts are changed
There was a problem hiding this comment.
@haiqi96 , we added an issue to remove this change in the future. But I did test that it works.
There was a problem hiding this comment.
it's not elegant but I don't think it's really an issue.
|
For the PR title, how about |
…ce with Socket.IO. (y-scope#880)
Description
This PR is an updated version of #814. I couldn't push directly to that branch, so made a new PR.
PR sets up fastify plugin, which forwards clients requests to mongoDB server, then responds with data.
Checklist
breaking change.
Validation performed
Ran the following tests
Checked proper subscription, and deallocation of resources when cursor was created and when component unmounts.
I tested in strict mode vs. non-strict mode. Lastly tested disconnection (different from unmounting).
See wip PR to test. Here is WIP PR which also includes client: #865
Summary by CodeRabbit
Summary by CodeRabbit