fix: support multiple WebSocket channels with query parameters #1973#2106
fix: support multiple WebSocket channels with query parameters #1973#2106SushanthMusham wants to merge 5 commits into
Conversation
SushanthMusham
requested review from
Adi-204,
asyncapi-bot-eve,
derberg,
jonaslagoni and
magicmatatjahu
as code owners
|
What reviewer looks at during PR reviewThe following are ideal points maintainers look for during review. Reviewing these points yourself beforehand can help streamline the review process and reduce time to merge.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactored WebSocket query parameter extraction to handle multiple AsyncAPI channels. Added ChangesMulti-channel WebSocket Query Parameters
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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. Comment |
SushanthMusham
changed the title
SushanthMusham
changed the title
SushanthMusham
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/helpers/src/bindings.js`:
- Around line 60-71: The code currently treats an empty query.properties ({}) as
present and creates an empty channelParams Map which breaks the "no query params
=> null" contract; modify the loop that processes query.properties in
bindings.js (the block that reads const properties = query.properties and sets
channelParams then allChannelsParams.set(channelName, channelParams)) to skip
when properties is an empty object by checking e.g. if (!properties || typeof
properties !== 'object' || Object.keys(properties).length === 0) continue, and
apply the same empty-object check to the corresponding block at the other
occurrence (lines around the second block that sets channelParams at 118-123) so
channels with {} produce no entry (preserving null-on-empty behavior).
- Around line 4-6: Update the JSDoc for the function that accepts the channels
parameter to reflect the actual expected type (an object with isEmpty() and
all() methods, e.g. an AsyncAPI channels container or a Map-like wrapper)
instead of Map<string,string>, and correct the `@returns` type if needed to match
the actual returned structure (Map<string, Map<string,string>> or appropriate).
Add an `@throws` (or `@throws` {TypeError}) clause documenting that the function
will throw when channels is null/undefined or when channels.isEmpty() is true
(or when channels.all() is not a function), and mention the exact conditions
that cause the error. Apply the same JSDoc fixes to the other occurrences noted
around lines 81-83 so parameter/return types and error conditions are consistent
for the functions using channels, channels.isEmpty(), and channels.all().
In `@packages/helpers/src/index.js`:
- Line 17: The new exported helper getQueryParamsForAllChannels lacks unit
tests; add a test that parses a fixture and invokes getQueryParamsForAllChannels
with the real Parser instance (per project convention) to validate its output
across expected channels. Create a test in the helpers test suite that loads a
representative fixture from the fixtures folder, constructs/uses the real
Parser, calls getQueryParamsForAllChannels, and asserts the returned query param
objects (including edge cases you expect). Ensure the test imports the helper by
name (getQueryParamsForAllChannels) and follows existing test patterns in the
helpers package.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0eaf2a35-c6c5-45b0-aab8-b2f9675f429d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
packages/helpers/src/bindings.jspackages/helpers/src/index.js
| * @param {Map<string,string>} channels - A Map representing all AsyncAPI channels. | ||
| * @returns {Map<string,string>} A Map whose keys are parameter names and whose values are their defaults (or `''`). | ||
| * @returns {Map<string, Map<string,string>>} A Map where keys are channel names and values are Maps of parameter names to their defaults (or `''`). | ||
| * |
There was a problem hiding this comment.
Fix JSDoc parameter typing and document error conditions accurately.
The channels type is documented as Map<string,string>, but the code requires an object with isEmpty() and all(). Please update the JSDoc types and add explicit error-condition docs (@throws or equivalent behavior note).
As per coding guidelines **/*.{js,ts,jsx,tsx}: Write clear and consistent JSDoc comments for functions, including parameter types, return values, and error conditions.
Also applies to: 81-83
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/helpers/src/bindings.js` around lines 4 - 6, Update the JSDoc for
the function that accepts the channels parameter to reflect the actual expected
type (an object with isEmpty() and all() methods, e.g. an AsyncAPI channels
container or a Map-like wrapper) instead of Map<string,string>, and correct the
`@returns` type if needed to match the actual returned structure (Map<string,
Map<string,string>> or appropriate). Add an `@throws` (or `@throws` {TypeError})
clause documenting that the function will throw when channels is null/undefined
or when channels.isEmpty() is true (or when channels.all() is not a function),
and mention the exact conditions that cause the error. Apply the same JSDoc
fixes to the other occurrences noted around lines 81-83 so parameter/return
types and error conditions are consistent for the functions using channels,
channels.isEmpty(), and channels.all().
| getServerProtocol, | ||
| listFiles, | ||
| getQueryParams, | ||
| getQueryParamsForAllChannels, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add dedicated tests for the newly exported helper.
getQueryParamsForAllChannels is now publicly exported, but this PR does not include corresponding helper tests. Please add coverage in packages/helpers/test/**/*.test.js using fixture parsing + real Parser per project convention.
As per coding guidelines {packages/helpers/src/**/*.js,packages/helpers/test/**/*.test.js}: Every exported helper in packages/helpers needs a test. Default to parsing a fixture from test/__fixtures__/ and passing the real Parser object to the helper.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/helpers/src/index.js` at line 17, The new exported helper
getQueryParamsForAllChannels lacks unit tests; add a test that parses a fixture
and invokes getQueryParamsForAllChannels with the real Parser instance (per
project convention) to validate its output across expected channels. Create a
test in the helpers test suite that loads a representative fixture from the
fixtures folder, constructs/uses the real Parser, calls
getQueryParamsForAllChannels, and asserts the returned query param objects
(including edge cases you expect). Ensure the test imports the helper by name
(getQueryParamsForAllChannels) and follows existing test patterns in the helpers
package.
|
@Adi-204 , I have cleared the two issues raised by the code rabbit , the tests part is what left, I am trying to do it as well , it is taking a bit time, I new to writing test, nevertheless will commit the new changes by tonight ,nvm , also the jsdoc is also wrong in the current code as well , Its an object from the very first but the current comment says its a map , fixed this too |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/helpers/test/bindings.test.js`:
- Around line 91-98: Add a JSDoc block above
getQueryParamsForAllChannelsFiltered describing the channelNames parameter and
the function return contract: document that channelNames is an array of strings
(string[]) and that the function returns the same Map-like structure produced by
createChannelsWithOnly/getQueryParamsForAllChannels (e.g., a Map of channelName
=> channelQueryParams or an object with size/all methods), including the shape
of the values (what each entry contains) and any error/edge behavior (empty
array -> empty map). Reference the function name
getQueryParamsForAllChannelsFiltered, the param channelNames, and the returned
filteredChannelsMap/getQueryParamsForAllChannels result in the JSDoc.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a9b6668e-f066-4f00-af55-11c45febb50c
📒 Files selected for processing (2)
packages/helpers/src/bindings.jspackages/helpers/test/bindings.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/helpers/src/bindings.js
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/helpers/test/bindings.test.js (1)
90-102:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd JSDoc for the new local helper function.
getFilteredAllChannelsParamsis newly/actively modified but undocumented. Please add JSDoc forchannelNames, return shape, and empty-input behavior.Proposed update
+/** + * Filters document channels by name and returns extracted WS query params for all matching channels. + * + * `@param` {string[]} channelNames - Channel IDs to include. + * `@returns` {Map<string, Map<string, string>>} Map of channel name -> query param defaults. + * Returns an empty Map when no matching channels with WS query properties are found. + */ const getFilteredAllChannelsParams = (channelNames) => { const channels = parsedAsyncAPIDocument.channels(); const filteredMap = new Map();As per coding guidelines, "
**/*.{js,ts,jsx,tsx}: Write clear and consistent JSDoc comments for functions, including parameter types, return values, and error conditions".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/helpers/test/bindings.test.js` around lines 90 - 102, Add a JSDoc block for the local helper function getFilteredAllChannelsParams describing the channelNames parameter (Array<string> of channel IDs to include), the return shape (the value returned by getQueryParamsForAllChannels: an object exposing isEmpty() and all() with all() returning an array of channel objects), and the empty-input behavior (when channelNames is empty or none match, isEmpty() should return true and all() should return an empty array). Place the comment immediately above the getFilteredAllChannelsParams declaration and reference parsedAsyncAPIDocument.channels() and getQueryParamsForAllChannels in the description so readers understand where channels are sourced and what is returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/helpers/test/bindings.test.js`:
- Around line 90-102: Add a JSDoc block for the local helper function
getFilteredAllChannelsParams describing the channelNames parameter
(Array<string> of channel IDs to include), the return shape (the value returned
by getQueryParamsForAllChannels: an object exposing isEmpty() and all() with
all() returning an array of channel objects), and the empty-input behavior (when
channelNames is empty or none match, isEmpty() should return true and all()
should return an empty array). Place the comment immediately above the
getFilteredAllChannelsParams declaration and reference
parsedAsyncAPIDocument.channels() and getQueryParamsForAllChannels in the
description so readers understand where channels are sourced and what is
returned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b2972e73-2b7f-4e16-8ed7-56e8d0362d29
📒 Files selected for processing (2)
packages/helpers/src/bindings.jspackages/helpers/test/bindings.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/helpers/src/bindings.js
|
|
@Adi-204 . |
2 participants
Problem:
The getQueryParams() helper function in packages/helpers/src/bindings.js currently assumes that only one channel has WebSocket bindings. This limitation prevents AsyncAPI documents with multiple channels from properly exposing query parameters for all channels.
Example scenario from issue #1973:
With the current implementation, only the first channel's query parameters are returned, completely ignoring notifications and alerts channels.
Solution Overview
The refactoring introduces a new helper function and restructures the existing one to support multiple channels while maintaining 100% backward compatibility.
Code Changes
1. New Function: getQueryParamsForAllChannels(channels)
Purpose: Extracts query parameters from ALL channels with WebSocket bindings
Return Type: Map<string, Map<string,string>> - Nested map structure
Behavior:
Example:
2. Refactored Function: getQueryParams(channels)
Purpose: Maintains backward compatibility by returning only the FIRST channel's parameters
Implementation: Now internally uses getQueryParamsForAllChannels() and returns the first entry
Behavior: Identical to original implementation - returns first channel's params or null
Breaking Changes: NONE - completely backward compatible
Example:
3. Updated Exports: packages/helpers/src/index.js
Key Design Decisions
Benefits
getQueryParamsForAllChannels()if neededTest Results
All existing tests pass with 100% coverage for
bindings.js:Files Modified
packages/helpers/src/bindings.js- Added new function, refactored existing functionpackages/helpers/src/index.js- Added new exportMigration Guide (Optional for consumers)
Before (still works)
After (new capability)
Related issue(s)
getQueryParamshelper function assumes only one channel has ws bindingsSummary by CodeRabbit
New Features
Tests