Update and optimize use of ws library in socks
#1409
Conversation
WalkthroughThe updates mainly involve adding the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
BrendanChou
force-pushed
the
bc/socks-ws
branch
from
efa1a70 to
7640378
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
indexer/package.jsonis excluded by!**/*.jsonindexer/pnpm-lock.yamlis excluded by!**/*.yaml
Files selected for processing (2)
- indexer/patches/@types+ws+8.5.10.patch (1 hunks)
- indexer/services/socks/src/helpers/wss.ts (1 hunks)
Additional Context Used
Path-based Instructions (2)
indexer/patches/@types+ws+8.5.10.patch (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/services/socks/src/helpers/wss.ts (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.
Additional comments not posted (2)
indexer/patches/@types+ws+8.5.10.patch (1)
16-19: The patch correctly introducesallowSynchronousEventsandautoPongproperties to the WebSocket server options.Verification successful
The new properties
allowSynchronousEventsandautoPonghave been successfully integrated into theserverOptionsobject in thewss.tsfile.
indexer/services/socks/src/helpers/wss.ts:
allowSynchronousEvents: falseautoPong: trueScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of the new properties in the WebSocket server implementation. # Test: Search for the usage of the new properties in the WebSocket server initialization. rg --type typescript $'new WebSocket.Server'Length of output: 80
Script:
#!/bin/bash # Description: Verify the integration of the new properties in the WebSocket server implementation. # Test: Search for the usage of the new properties in the WebSocket server initialization. rg --type ts 'new WebSocket.Server'Length of output: 131
Script:
#!/bin/bash # Description: Verify the integration of the new properties in the WebSocket server implementation. # Test: Search for the definition of the `serverOptions` object and its properties in the `wss.ts` file. rg 'serverOptions' indexer/services/socks/src/helpers/wss.tsLength of output: 165
Script:
#!/bin/bash # Description: Verify the integration of the new properties in the WebSocket server implementation. # Test: Display the contents of the `serverOptions` object in the `wss.ts` file. rg -A 10 'const serverOptions: WebSocket.ServerOptions =' indexer/services/socks/src/helpers/wss.tsLength of output: 415
indexer/services/socks/src/helpers/wss.ts (1)
21-25: The WebSocket server options have been updated to includeautoPong: true. This aligns with the PR's objectives to simplify ping/pong handling.
| port: config.WS_PORT, | ||
| }); | ||
| allowSynchronousEvents: false, |
There was a problem hiding this comment.
Consider setting allowSynchronousEvents to true as per the PR's performance improvement plan.
- allowSynchronousEvents: false,
+ allowSynchronousEvents: 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.
| allowSynchronousEvents: false, | |
| allowSynchronousEvents: true, |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
indexer/pnpm-lock.yamlis excluded by!**/*.yaml
Files selected for processing (5)
- indexer/Dockerfile.auxo.remote (1 hunks)
- indexer/Dockerfile.bazooka.remote (1 hunks)
- indexer/Dockerfile.postgres-package.local (1 hunks)
- indexer/Dockerfile.service.local (1 hunks)
- indexer/Dockerfile.service.remote (1 hunks)
Files skipped from review due to trivial changes (1)
- indexer/Dockerfile.postgres-package.local
Additional Context Used
Path-based Instructions (4)
indexer/Dockerfile.auxo.remote (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/Dockerfile.bazooka.remote (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/Dockerfile.service.local (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/Dockerfile.service.remote (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.
Additional comments not posted (4)
indexer/Dockerfile.auxo.remote (1)
22-22: The use of--loglevel warn,--frozen-lockfile, and--unsafe-permflags in thepnpm installcommand is appropriate for ensuring a stable and secure build environment.indexer/Dockerfile.bazooka.remote (1)
25-25: Consistent use ofpnpm installflags across different service Dockerfiles helps maintain a uniform build process.indexer/Dockerfile.service.local (1)
45-45: Using--productionflag in development environment installations is a good practice to mirror production settings, helping catch potential deployment issues early.indexer/Dockerfile.service.remote (1)
45-45: Maintaining consistency in dependency installation commands between local and remote environments is crucial for ensuring that services behave similarly in both contexts.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
indexer/pnpm-lock.yamlis excluded by!**/*.yamlindexer/services/socks/package.jsonis excluded by!**/*.json
Files selected for processing (5)
- indexer/Dockerfile.auxo.remote (2 hunks)
- indexer/Dockerfile.bazooka.remote (2 hunks)
- indexer/Dockerfile.postgres-package.local (2 hunks)
- indexer/Dockerfile.service.local (2 hunks)
- indexer/Dockerfile.service.remote (2 hunks)
Files skipped from review as they are similar to previous changes (5)
- indexer/Dockerfile.auxo.remote
- indexer/Dockerfile.bazooka.remote
- indexer/Dockerfile.postgres-package.local
- indexer/Dockerfile.service.local
- indexer/Dockerfile.service.remote
| @@ -22,7 +25,7 @@ COPY tsconfig.json ./ | |||
| RUN npm install -g pnpm@6 | |||
|
|
|||
| # Install npm modules using pnpm | |||
| RUN pnpm install --loglevel warn --frozen-lockfile | |||
| RUN pnpm install --loglevel warn --frozen-lockfile --unsafe-perm | |||
There was a problem hiding this comment.
--unsafe-perm needed, otherwise npm skips the postinstall since it thinks its malicious
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- indexer/Dockerfile.auxo.remote (2 hunks)
- indexer/Dockerfile.bazooka.remote (2 hunks)
- indexer/Dockerfile.postgres-package.local (2 hunks)
- indexer/Dockerfile.service.local (2 hunks)
- indexer/Dockerfile.service.remote (2 hunks)
- indexer/services/socks/src/helpers/wss.ts (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- indexer/Dockerfile.auxo.remote
- indexer/Dockerfile.bazooka.remote
- indexer/Dockerfile.postgres-package.local
- indexer/Dockerfile.service.local
- indexer/Dockerfile.service.remote
- indexer/services/socks/src/helpers/wss.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
indexer/services/socks/__tests__/websocket/index.test.ts (3)
Line range hint
5-5: Import WebSocket with thenode:protocol.- import WebSocket from 'ws'; + import WebSocket from 'node:ws';This change is recommended to explicitly distinguish Node.js built-in modules from npm packages, improving clarity and potentially preventing naming conflicts.
Line range hint
6-6: Import other Node.js built-in modules with thenode:protocol.- import { IncomingMessage } from 'http'; + import { IncomingMessage } from 'node:http';This change follows best practices for importing built-in modules in Node.js, ensuring consistency and clarity in imports.
Line range hint
321-321: Specify a more precise type instead ofany.- function createIncomingMessage(properties: any): IncomingMessage { + function createIncomingMessage(properties: Record<string, unknown>): IncomingMessage {Using
Record<string, unknown>instead ofanyprovides better type safety by enforcing thatpropertiesis an object with string keys, which aligns with TypeScript's best practices for type safety.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- indexer/services/socks/tests/websocket/index.test.ts (2 hunks)
Additional context used
Path-based instructions (1)
indexer/services/socks/__tests__/websocket/index.test.ts (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.
Biome
indexer/services/socks/__tests__/websocket/index.test.ts
[error] 5-5: A Node.js builtin module should be imported with the node: protocol.
[error] 6-6: A Node.js builtin module should be imported with the node: protocol.
[error] 321-321: Unexpected any. Specify a different type.
Additional comments not posted (2)
indexer/services/socks/__tests__/websocket/index.test.ts (2)
52-52: Updated WebSocket constructor to include additional parameters.This change aligns with the PR's objective to handle WebSocket connections more robustly by explicitly setting options, even if they are empty.
267-267: Adjusted expectation forwsPongSpycall count.This change reflects the updated behavior due to the
autoPong: truesetting in the WebSocket server, which now handles pongs automatically, reducing the need for explicit pong handling in tests.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- indexer/services/socks/tests/websocket/index.test.ts (4 hunks)
Additional context used
Path-based instructions (1)
indexer/services/socks/__tests__/websocket/index.test.ts (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.
Biome
indexer/services/socks/__tests__/websocket/index.test.ts
[error] 5-5: A Node.js builtin module should be imported with the node: protocol.
[error] 6-6: A Node.js builtin module should be imported with the node: protocol.
[error] 51-51: Unexpected any. Specify a different type.
[error] 309-309: Unexpected any. Specify a different type.
Additional comments not posted (1)
indexer/services/socks/__tests__/websocket/index.test.ts (1)
75-75: Removal of thewsPongSpycall count expectation is appropriate given the automatic pong handling by the updatedwslibrary.
| @@ -49,10 +48,9 @@ describe('Index', () => { | |||
| (Subscriptions as unknown as jest.Mock).mockClear(); | |||
| (sendMessage as unknown as jest.Mock).mockClear(); | |||
| mockWss = new Wss(); | |||
| websocket = new WebSocket(null); | |||
| websocket = new WebSocket(null, [], { autoPong: true } as any); | |||
There was a problem hiding this comment.
Update to WebSocket constructor to include autoPong: true aligns with PR objectives. However, avoid using as any for type safety.
- websocket = new WebSocket(null, [], { autoPong: true } as any);
+ // Define a proper type for the options or extend existing types to include autoPong
+ websocket = new WebSocket(null, [], { autoPong: true } as WebSocketOptions);Committable suggestion was skipped due low confidence.
Tools
Biome
[error] 51-51: Unexpected any. Specify a different type.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
indexer/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (1)
- indexer/services/socks/tests/websocket/index.test.ts (4 hunks)
Files skipped from review due to trivial changes (1)
- indexer/services/socks/tests/websocket/index.test.ts
Changelist
wslibrary insocksservice from8.8.1to8.16.0.pingas thewslibrary handles this by default. Explicitly setautoPong: truein server initialization to show thisallowSynchronousEventstotruewhich will be the default value in the next version ofws(commit here). This is expected to improve performanceTest Plan
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
Bug Fixes
Chores
--unsafe-permflag.