refactor: update getQueryParams to support multiple channels#2026
refactor: update getQueryParams to support multiple channels#2026SushanthMusham wants to merge 7 commits intoasyncapi:masterfrom
Conversation
…template compatibility
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughRefactors query-parameter helpers to collect WS query properties per channel, adds and exports getFirstChannelQueryParams (returns first channel’s params as a Map), and updates templates, components, docs, and tests to consume the new per-channel shape or the first-channel Map. No public API signatures were removed. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
packages/templates/clients/websocket/python/test/components/QueryParamsArgumentsDocs.test.js (1)
23-26: Guard empty-channel results before first-channel extraction.If
getQueryParamsreturns an empty object, Line 24 producesundefinedand Line 25 will throw onObject.entries(undefined). Add a key-length check before indexing.Suggested fix
let queryParamsArray = null; -if (allQueryParams) { - const firstChannelName = Object.keys(allQueryParams)[0]; - queryParamsArray = Object.entries(allQueryParams[firstChannelName]); +if (allQueryParams && Object.keys(allQueryParams).length > 0) { + const firstChannelName = Object.keys(allQueryParams)[0]; + const firstChannelQueryParams = allQueryParams[firstChannelName]; + queryParamsArray = firstChannelQueryParams ? Object.entries(firstChannelQueryParams) : null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/templates/clients/websocket/python/test/components/QueryParamsArgumentsDocs.test.js` around lines 23 - 26, The code assumes allQueryParams has at least one key and does Object.keys(allQueryParams)[0] then Object.entries(...) which will throw if allQueryParams is empty; update the block that sets firstChannelName and queryParamsArray to first check that Object.keys(allQueryParams).length > 0 (or that firstChannelName is truthy) before indexing and calling Object.entries, and only set queryParamsArray when a channel exists (leave it empty or default otherwise) so references to firstChannelName and queryParamsArray are safe; target the variables allQueryParams, firstChannelName and queryParamsArray in the test.packages/templates/clients/websocket/python/test/components/Requires.test.js (1)
20-27: Consider adding a defensive check for emptyallQueryParams.If
getQueryParamsreturns an empty object{},Object.keys(allQueryParams)[0]will beundefined, causingObject.entries(allQueryParams[undefined])to throw aTypeError. While the fixture presumably contains query params, a small guard would make this more robust:🛡️ Proposed defensive fix
const allQueryParams = getQueryParams(channels); // Convert the first channel's parameters back into a Map so the test doesn't break let queryParams = null; -if (allQueryParams) { +if (allQueryParams && Object.keys(allQueryParams).length > 0) { const firstChannelName = Object.keys(allQueryParams)[0]; queryParams = new Map(Object.entries(allQueryParams[firstChannelName])); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/templates/clients/websocket/python/test/components/Requires.test.js` around lines 20 - 27, The test assumes getQueryParams(channels) returns a non-empty object and directly accesses Object.keys(allQueryParams)[0], which can be undefined; update the block around allQueryParams in Requires.test.js to defensively handle an empty result by checking that allQueryParams is truthy and has at least one key before accessing the first channel (e.g., use const keys = Object.keys(allQueryParams); if (allQueryParams && keys.length > 0) { const firstChannelName = keys[0]; queryParams = new Map(Object.entries(allQueryParams[firstChannelName] || {})); } ), ensuring you reference getQueryParams, allQueryParams, and queryParams in the change.packages/templates/clients/websocket/python/components/ClientClass.js (1)
10-71: Consider adding JSDoc for the exported function.The
ClientClassfunction is exported but lacks JSDoc documentation. As per coding guidelines, functions should include clear JSDoc comments with parameter types and return values.📝 Suggested JSDoc addition
+/** + * Renders the Python WebSocket client class. + * + * `@param` {Object} props - Component props. + * `@param` {Object} props.asyncapi - The parsed AsyncAPI document. + * `@param` {Object} props.params - Template parameters including server, appendClientSuffix, and customClientName. + * `@returns` {JSX.Element} The rendered Python client class. + */ export function ClientClass({ asyncapi, params }) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/templates/clients/websocket/python/components/ClientClass.js` around lines 10 - 71, Add a JSDoc block above the exported function ClientClass that documents the function, its parameters and return type: describe the first param asyncapi as the AsyncAPI document/object (type AsyncAPI or any), the second param params as an options object documenting expected fields (e.g. server, appendClientSuffix, customClientName), and state the function returns a JSX/React/Text element (or string template fragment) used to render the client class; reference the function name ClientClass and the parameter names asyncapi and params so the comment is placed directly above export function ClientClass({ asyncapi, params }) and follows project JSDoc style.packages/templates/clients/websocket/java/quarkus/template/src/main/java/com/asyncapi/connector.java.js (2)
6-30: Consider adding JSDoc for the exported function.The default exported async function lacks JSDoc documentation describing its parameters and return value.
📝 Suggested JSDoc addition
+/** + * Generates the Java Quarkus WebSocket connector file. + * + * `@param` {Object} props - Generator props. + * `@param` {Object} props.asyncapi - The parsed AsyncAPI document. + * `@param` {Object} props.params - Template parameters including server, appendClientSuffix, and customClientName. + * `@returns` {Promise<JSX.Element>} The rendered connector file. + */ export default async function ({ asyncapi, params }) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/templates/clients/websocket/java/quarkus/template/src/main/java/com/asyncapi/connector.java.js` around lines 6 - 30, Add a JSDoc block above the default exported async function to document the input parameters and return value: describe the function as async and list the shape/meaning of the single destructured parameter ({ asyncapi, params }) including expected params fields (server, appendClientSuffix, customClientName), note that it uses helpers getServer/getClientName/getQueryParams and returns a JSX File element containing ConnectorDependencies and ClientConnector (i.e., a template fragment), and document any thrown errors or side effects if applicable.
9-18: Consider extracting the repeated bridge logic into a shared helper.The same "first channel to Map" conversion pattern is duplicated across multiple template files (Python
ClientClass.js, Javaconnector.java.js,client.java.js, etc.). Consider adding a utility function in@asyncapi/generator-helpersto reduce duplication:// Example helper in generator-helpers function getFirstChannelQueryParamsAsMap(channels) { const allQueryParams = getQueryParams(channels); if (!allQueryParams) return null; const firstChannelName = Object.keys(allQueryParams)[0]; return new Map(Object.entries(allQueryParams[firstChannelName])); }This would simplify all consuming code to a single function call while preserving backward compatibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/templates/clients/websocket/java/quarkus/template/src/main/java/com/asyncapi/connector.java.js` around lines 9 - 18, Extract the duplicated "first channel to Map" logic into a shared helper called getFirstChannelQueryParamsAsMap in `@asyncapi/generator-helpers`: implement a function that calls getQueryParams(channels), returns null if falsy, otherwise picks the first channel key and returns new Map(Object.entries(...)); then replace the inline pattern in connector.java.js (the allQueryParams variable and subsequent Map construction that sets queryParams) with a single call to getFirstChannelQueryParamsAsMap(asyncapi.channels()) to keep behavior identical and remove duplication.packages/components/test/components/QueryParamsVariables.test.js (1)
22-26: Consider extracting the repeated first-channel conversion into a test helper.The same block appears 3 times; centralizing it will reduce drift and keep future shape changes easier to apply.
♻️ Refactor sketch
+function getFirstChannelQueryParamsArray(channels) { + const queryParamsObject = getQueryParams(channels); + if (!queryParamsObject || Object.keys(queryParamsObject).length === 0) return []; + const firstChannelName = Object.keys(queryParamsObject)[0]; + return Object.entries(queryParamsObject[firstChannelName] ?? {}); +} ... - const queryParamsObject = getQueryParams(channels); - let queryParamsArray = []; - if (queryParamsObject) { - const firstChannelName = Object.keys(queryParamsObject)[0]; - queryParamsArray = Object.entries(queryParamsObject[firstChannelName]); - } + const queryParamsArray = getFirstChannelQueryParamsArray(channels);Also applies to: 41-45, 60-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/components/test/components/QueryParamsVariables.test.js` around lines 22 - 26, Extract the repeated conversion logic for test fixtures into a small test helper (e.g., getQueryParamsArray) and replace the three identical blocks that compute queryParamsArray from queryParamsObject using firstChannelName/ Object.entries with a single call to that helper; the helper should accept queryParamsObject, guard for falsy input, compute Object.keys(queryParamsObject)[0] as firstChannelName, and return Object.entries(queryParamsObject[firstChannelName]) so tests use a single source of truth for this transformation.packages/templates/clients/websocket/java/quarkus/test/components/InitConnector.test.js (1)
21-30: LGTM with optional defensive improvement.The two-step extraction pattern correctly adapts to the new multi-channel
getQueryParamsAPI. For test code with a known fixture, this is acceptable.Optional: Add a guard for the empty object edge case if you want the pattern to be more robust for future reuse:
🛡️ Optional defensive guard
// Extract the parameters from the first channel into an array for the tests queryParamsArray = null; - if (allQueryParams) { + if (allQueryParams && Object.keys(allQueryParams).length > 0) { const firstChannelName = Object.keys(allQueryParams)[0]; queryParamsArray = Object.entries(allQueryParams[firstChannelName]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/templates/clients/websocket/java/quarkus/test/components/InitConnector.test.js` around lines 21 - 30, The test currently assumes allQueryParams has at least one channel before reading Object.keys(allQueryParams)[0]; add a defensive guard around the extraction of firstChannelName and building queryParamsArray: check that allQueryParams is an object and has at least one key (non-empty) before calling Object.keys(...) and Object.entries(...), otherwise set queryParamsArray to an empty array or a sensible default; update the code around getQueryParams, allQueryParams, firstChannelName, and queryParamsArray to handle the empty-object case safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/components/src/components/QueryParamsVariables.js`:
- Around line 116-145: The JSDoc example for the QueryParamsVariables component
contains malformed comment markers (many instances of "* *") which breaks
rendering; edit the JSDoc example block for QueryParamsVariables in
QueryParamsVariables.js and replace all occurrences of "* *" at the start of
example lines with a single "*" so each example line is a proper JSDoc line
(preserve spacing/indentation and all example content, only fix the duplicated
asterisks).
In `@packages/helpers/src/bindings.js`:
- Around line 14-59: Update the JSDoc for getQueryParams to specify that the
channels parameter is an AsyncAPI Channels collection (not a plain Object) that
implements .isEmpty() and .all(), and document assumptions and error conditions:
note that the function expects channels to be non-null and to provide .isEmpty()
and .all() methods (or it will return null/skip processing), and describe return
values (object mapping channelName -> params or null when no ws query params
found). Reference getQueryParams(channels), the channels .isEmpty() and .all()
usage, and the expectation that channel bindings provide a ws binding with
.value()?.query.
In
`@packages/templates/clients/websocket/java/quarkus/template/src/main/java/com/asyncapi/client.java.js`:
- Around line 17-20: The current block uses Object.keys(allQueryParams)[0]
without ensuring there is at least one key, which can throw; update the guard
around allQueryParams to also check that Object.keys(allQueryParams).length > 0
and that the resolved firstChannelName entry exists before converting to a Map.
Specifically, change the conditional that references
allQueryParams/firstChannelName and ensure queryParams = new
Map(Object.entries(allQueryParams[firstChannelName])) only runs when a non-empty
key list exists and allQueryParams[firstChannelName] is truthy.
In `@packages/templates/clients/websocket/python/template/client.py.js`:
- Around line 16-19: The template assumes allQueryParams has at least one key
and does Map(Object.entries(...)) unguarded; add a guard to ensure a
firstChannelName and a defined allQueryParams[firstChannelName] before
converting to a Map and otherwise set queryParams to an empty Map. Specifically,
update the block that references allQueryParams, firstChannelName and
queryParams so it checks Object.keys(allQueryParams).length > 0 (or that
firstChannelName is truthy and allQueryParams[firstChannelName] !== undefined)
before calling Object.entries, and fallback to new Map() when there are no
entries.
In
`@packages/templates/clients/websocket/python/test/components/InitSignature.test.js`:
- Around line 60-63: The code assumes allQueryParams has at least one key before
calling Object.entries and can throw when allQueryParams is an empty object;
change the guard to ensure there is at least one channel and that the first
channel's params exist before calling Object.entries (e.g., check
Object.keys(allQueryParams).length > 0 and that allQueryParams[firstChannelName]
is defined) so queryParamsArray is only assigned from Object.entries when a
valid object exists (referencing allQueryParams, firstChannelName, and
queryParamsArray in InitSignature.test.js).
---
Nitpick comments:
In `@packages/components/test/components/QueryParamsVariables.test.js`:
- Around line 22-26: Extract the repeated conversion logic for test fixtures
into a small test helper (e.g., getQueryParamsArray) and replace the three
identical blocks that compute queryParamsArray from queryParamsObject using
firstChannelName/ Object.entries with a single call to that helper; the helper
should accept queryParamsObject, guard for falsy input, compute
Object.keys(queryParamsObject)[0] as firstChannelName, and return
Object.entries(queryParamsObject[firstChannelName]) so tests use a single source
of truth for this transformation.
In
`@packages/templates/clients/websocket/java/quarkus/template/src/main/java/com/asyncapi/connector.java.js`:
- Around line 6-30: Add a JSDoc block above the default exported async function
to document the input parameters and return value: describe the function as
async and list the shape/meaning of the single destructured parameter ({
asyncapi, params }) including expected params fields (server,
appendClientSuffix, customClientName), note that it uses helpers
getServer/getClientName/getQueryParams and returns a JSX File element containing
ConnectorDependencies and ClientConnector (i.e., a template fragment), and
document any thrown errors or side effects if applicable.
- Around line 9-18: Extract the duplicated "first channel to Map" logic into a
shared helper called getFirstChannelQueryParamsAsMap in
`@asyncapi/generator-helpers`: implement a function that calls
getQueryParams(channels), returns null if falsy, otherwise picks the first
channel key and returns new Map(Object.entries(...)); then replace the inline
pattern in connector.java.js (the allQueryParams variable and subsequent Map
construction that sets queryParams) with a single call to
getFirstChannelQueryParamsAsMap(asyncapi.channels()) to keep behavior identical
and remove duplication.
In
`@packages/templates/clients/websocket/java/quarkus/test/components/InitConnector.test.js`:
- Around line 21-30: The test currently assumes allQueryParams has at least one
channel before reading Object.keys(allQueryParams)[0]; add a defensive guard
around the extraction of firstChannelName and building queryParamsArray: check
that allQueryParams is an object and has at least one key (non-empty) before
calling Object.keys(...) and Object.entries(...), otherwise set queryParamsArray
to an empty array or a sensible default; update the code around getQueryParams,
allQueryParams, firstChannelName, and queryParamsArray to handle the
empty-object case safely.
In `@packages/templates/clients/websocket/python/components/ClientClass.js`:
- Around line 10-71: Add a JSDoc block above the exported function ClientClass
that documents the function, its parameters and return type: describe the first
param asyncapi as the AsyncAPI document/object (type AsyncAPI or any), the
second param params as an options object documenting expected fields (e.g.
server, appendClientSuffix, customClientName), and state the function returns a
JSX/React/Text element (or string template fragment) used to render the client
class; reference the function name ClientClass and the parameter names asyncapi
and params so the comment is placed directly above export function ClientClass({
asyncapi, params }) and follows project JSDoc style.
In
`@packages/templates/clients/websocket/python/test/components/QueryParamsArgumentsDocs.test.js`:
- Around line 23-26: The code assumes allQueryParams has at least one key and
does Object.keys(allQueryParams)[0] then Object.entries(...) which will throw if
allQueryParams is empty; update the block that sets firstChannelName and
queryParamsArray to first check that Object.keys(allQueryParams).length > 0 (or
that firstChannelName is truthy) before indexing and calling Object.entries, and
only set queryParamsArray when a channel exists (leave it empty or default
otherwise) so references to firstChannelName and queryParamsArray are safe;
target the variables allQueryParams, firstChannelName and queryParamsArray in
the test.
In
`@packages/templates/clients/websocket/python/test/components/Requires.test.js`:
- Around line 20-27: The test assumes getQueryParams(channels) returns a
non-empty object and directly accesses Object.keys(allQueryParams)[0], which can
be undefined; update the block around allQueryParams in Requires.test.js to
defensively handle an empty result by checking that allQueryParams is truthy and
has at least one key before accessing the first channel (e.g., use const keys =
Object.keys(allQueryParams); if (allQueryParams && keys.length > 0) { const
firstChannelName = keys[0]; queryParams = new
Map(Object.entries(allQueryParams[firstChannelName] || {})); } ), ensuring you
reference getQueryParams, allQueryParams, and queryParams in the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2148922a-f2a0-4cea-ace3-8a4caf5969d3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
packages/components/src/components/QueryParamsVariables.jspackages/components/test/components/QueryParamsVariables.test.jspackages/helpers/src/bindings.jspackages/helpers/test/bindings.test.jspackages/templates/clients/websocket/java/quarkus/template/src/main/java/com/asyncapi/client.java.jspackages/templates/clients/websocket/java/quarkus/template/src/main/java/com/asyncapi/connector.java.jspackages/templates/clients/websocket/java/quarkus/test/components/ClientConnector.test.jspackages/templates/clients/websocket/java/quarkus/test/components/ClientDependencies.test.jspackages/templates/clients/websocket/java/quarkus/test/components/ClientFields.test.jspackages/templates/clients/websocket/java/quarkus/test/components/ConnectorDependencies.test.jspackages/templates/clients/websocket/java/quarkus/test/components/Constructor.test.jspackages/templates/clients/websocket/java/quarkus/test/components/ConstructorSignature.test.jspackages/templates/clients/websocket/java/quarkus/test/components/DefaultConstructorSignature.test.jspackages/templates/clients/websocket/java/quarkus/test/components/EchoWebSocket.test.jspackages/templates/clients/websocket/java/quarkus/test/components/InitConnector.test.jspackages/templates/clients/websocket/java/quarkus/test/components/URIParams.test.jspackages/templates/clients/websocket/python/components/ClientClass.jspackages/templates/clients/websocket/python/template/client.py.jspackages/templates/clients/websocket/python/test/components/Constructor.test.jspackages/templates/clients/websocket/python/test/components/InitSignature.test.jspackages/templates/clients/websocket/python/test/components/QueryParamsArgumentsDocs.test.jspackages/templates/clients/websocket/python/test/components/Requires.test.js
.../templates/clients/websocket/java/quarkus/template/src/main/java/com/asyncapi/client.java.js
Outdated
Show resolved
Hide resolved
packages/templates/clients/websocket/python/template/client.py.js
Outdated
Show resolved
Hide resolved
packages/templates/clients/websocket/python/test/components/InitSignature.test.js
Outdated
Show resolved
Hide resolved
Adi-204
left a comment
Adi-204
left a comment
There was a problem hiding this comment.
@SushanthMusham, the only concern I have with the current refactoring is the repeated pattern below:
// Fetch all channel parameters with our new helper
const allQueryParams = getQueryParams(channels);
// Convert the first channel's parameters back into a Map for the tests
queryParams = null;
if (allQueryParams) {
const firstChannelName = Object.keys(allQueryParams)[0];
queryParams = new Map(Object.entries(allQueryParams[firstChannelName]));
}This logic is now duplicated across multiple components due to the helper change, which makes the code a bit redundant and harder to maintain.
I think it would be better to introduce an additional helper that directly returns the parameters for the first channel (in the required format). That way, we can avoid repetition and keep the components cleaner.
Also address the coderabbit comments it is much better now and give nice suggestions.
|
I’ve completed the refactor as suggested.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/helpers/src/bindings.js`:
- Around line 19-55: The code uses plain object literals for document-controlled
keys which allows prototype pollution; replace the mutable plain objects with
null-prototype objects by creating result and the per-channel channelParams
using Object.create(null) instead of {} (references: result, channelParams,
channelName, properties, channels.all(), and the loop that assigns query
property names) so that assigning document-derived keys cannot mutate
Object.prototype.
🪄 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: 898b5c3d-e907-4d4d-82b2-42d1c74191b4
📒 Files selected for processing (17)
packages/components/src/components/QueryParamsVariables.jspackages/helpers/src/bindings.jspackages/helpers/src/index.jspackages/templates/clients/websocket/java/quarkus/template/src/main/java/com/asyncapi/client.java.jspackages/templates/clients/websocket/java/quarkus/template/src/main/java/com/asyncapi/connector.java.jspackages/templates/clients/websocket/java/quarkus/test/components/ClientConnector.test.jspackages/templates/clients/websocket/java/quarkus/test/components/ClientDependencies.test.jspackages/templates/clients/websocket/java/quarkus/test/components/ClientFields.test.jspackages/templates/clients/websocket/java/quarkus/test/components/ConnectorDependencies.test.jspackages/templates/clients/websocket/java/quarkus/test/components/Constructor.test.jspackages/templates/clients/websocket/java/quarkus/test/components/ConstructorSignature.test.jspackages/templates/clients/websocket/java/quarkus/test/components/DefaultConstructorSignature.test.jspackages/templates/clients/websocket/java/quarkus/test/components/EchoWebSocket.test.jspackages/templates/clients/websocket/java/quarkus/test/components/InitConnector.test.jspackages/templates/clients/websocket/java/quarkus/test/components/URIParams.test.jspackages/templates/clients/websocket/python/template/client.py.jspackages/templates/clients/websocket/python/test/components/InitSignature.test.js
✅ Files skipped from review due to trivial changes (4)
- packages/templates/clients/websocket/java/quarkus/test/components/Constructor.test.js
- packages/templates/clients/websocket/java/quarkus/test/components/URIParams.test.js
- packages/templates/clients/websocket/java/quarkus/test/components/ConnectorDependencies.test.js
- packages/components/src/components/QueryParamsVariables.js
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/templates/clients/websocket/java/quarkus/template/src/main/java/com/asyncapi/client.java.js
- packages/templates/clients/websocket/java/quarkus/test/components/EchoWebSocket.test.js
- packages/templates/clients/websocket/java/quarkus/test/components/ConstructorSignature.test.js
- packages/templates/clients/websocket/java/quarkus/test/components/DefaultConstructorSignature.test.js
- packages/templates/clients/websocket/java/quarkus/test/components/InitConnector.test.js
- packages/templates/clients/websocket/python/test/components/InitSignature.test.js
- packages/templates/clients/websocket/java/quarkus/test/components/ClientDependencies.test.js
- packages/templates/clients/websocket/python/template/client.py.js
- packages/templates/clients/websocket/java/quarkus/template/src/main/java/com/asyncapi/connector.java.js
|
@SushanthMusham test are failing please fix it before I can review code changes |
|
@Adi-204 I've sent a fix for the tests! It turns out that the changes I made to the code were fine. However, running npm run build on my computer accidentally hardcoded my absolute path into connector.java.js (this seems to have something to do with the bug in Issue #1897). I went back to the clean version of that file from master, and now all the CI tests are passing. Whenever you have time, it's ready for review! |
@SushanthMusham CI is still failing. A few quick pointers:
Let me know if you get stuck 👍 |
|
Hey @Adi-204, thanks for the feedback! I've pushed an update that addresses this. I created a new getFirstChannelQueryParams helper in bindings.js and refactored all the components to use it, completely removing the duplicated logic. I also addressed the CodeRabbit comments by switching to Object.entries() to avoid the prototype pollution issue, and updated the Jest test snapshots to reflect the changes. Let me know if everything looks good! |
|
🚀 Docs preview deployed |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/templates/clients/websocket/java/quarkus/template/src/main/java/com/asyncapi/connector.java.js (1)
6-9: Document the template entry-point contract.Line 9 changes this function to use the first-channel compatibility helper, but the exported template still has no JSDoc for
{ asyncapi, params }, its return value, or failure cases such as an invalid server lookup. Please add that contract here.As per coding guidelines, "Write clear and consistent JSDoc comments for functions, including parameter types, return values, and error conditions".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/templates/clients/websocket/java/quarkus/template/src/main/java/com/asyncapi/connector.java.js` around lines 6 - 9, Add a JSDoc block to the exported default async function (the template entry-point) that documents the parameter object shape ({ asyncapi, params }), describing types for asyncapi (AsyncAPI document object), params (object with server, appendClientSuffix, customClientName, etc.), and what values are expected; document the return value (Promise resolving to the generated template context or string) and any possible failure modes/errors (e.g., when getServer fails to find a server and throws, or when helpers getClientName/getFirstChannelQueryParams return invalid data). Mention which helpers can produce errors by name (getServer, getClientName, getFirstChannelQueryParams) so callers know to handle invalid server lookup and other failure cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/templates/clients/websocket/java/quarkus/components/ClientConnector.js`:
- Around line 5-6: ClientConnector currently references an undefined queryParams
variable; change the creation of queryParamsArray to derive from the passed-in
prop (e.g., use Array.from(query.entries()) when query is truthy) so rendering
won't throw, and add a JSDoc block above the ClientConnector function that
documents the props (clientName:string, query:
URLSearchParams|Map|string[][]|null, pathName:string, operations:object) and the
return value to make the expected prop shape explicit.
In
`@packages/templates/clients/websocket/java/quarkus/components/ClientFields.js`:
- Around line 4-6: ClientFields currently calls Object.entries(queryParams)
which returns [] when queryParams is a Map (getFirstChannelQueryParams returns a
Map and EchoWebSocket passes it), so query param fields are dropped; update
ClientFields to detect Map vs plain object and use queryParams.entries() wrapped
in Array.from when queryParams instanceof Map, otherwise use Object.entries, and
add a JSDoc comment on the ClientFields function documenting that queryParams
may be a Map<string,string> or a plain object Record<string,string> and describe
clientName type.
In `@packages/templates/clients/websocket/java/quarkus/components/Constructor.js`:
- Around line 6-7: The Constructor component currently uses an undefined
variable queryParams; change it to build the array from the incoming query prop
(e.g., const queryParamsArray = query && Array.from(query.entries())) and ensure
you reference the prop name `query` in that expression; also add a JSDoc block
above the exported function `Constructor({ clientName, query })` describing the
accepted props shape (clientName: string, query: URLSearchParams |
null/undefined) and the return value so the template consumer and linters have
clear types.
---
Nitpick comments:
In
`@packages/templates/clients/websocket/java/quarkus/template/src/main/java/com/asyncapi/connector.java.js`:
- Around line 6-9: Add a JSDoc block to the exported default async function (the
template entry-point) that documents the parameter object shape ({ asyncapi,
params }), describing types for asyncapi (AsyncAPI document object), params
(object with server, appendClientSuffix, customClientName, etc.), and what
values are expected; document the return value (Promise resolving to the
generated template context or string) and any possible failure modes/errors
(e.g., when getServer fails to find a server and throws, or when helpers
getClientName/getFirstChannelQueryParams return invalid data). Mention which
helpers can produce errors by name (getServer, getClientName,
getFirstChannelQueryParams) so callers know to handle invalid server lookup and
other failure cases.
🪄 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: f233ad61-b07e-4662-8c0a-4adc30717ef9
⛔ Files ignored due to path filters (1)
packages/components/test/components/__snapshots__/QueryParamsVariables.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (13)
apps/generator/docs/api_components.mdpackages/components/src/components/QueryParamsVariables.jspackages/components/test/components/QueryParamsVariables.test.jspackages/templates/clients/websocket/java/quarkus/components/ClientConnector.jspackages/templates/clients/websocket/java/quarkus/components/ClientFields.jspackages/templates/clients/websocket/java/quarkus/components/Constructor.jspackages/templates/clients/websocket/java/quarkus/template/src/main/java/com/asyncapi/connector.java.jspackages/templates/clients/websocket/python/components/ClientClass.jspackages/templates/clients/websocket/python/template/client.py.jspackages/templates/clients/websocket/python/test/components/Constructor.test.jspackages/templates/clients/websocket/python/test/components/InitSignature.test.jspackages/templates/clients/websocket/python/test/components/QueryParamsArgumentsDocs.test.jspackages/templates/clients/websocket/python/test/components/Requires.test.js
✅ Files skipped from review due to trivial changes (1)
- packages/components/src/components/QueryParamsVariables.js
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/components/test/components/QueryParamsVariables.test.js
- packages/templates/clients/websocket/python/test/components/Constructor.test.js
- packages/templates/clients/websocket/python/test/components/InitSignature.test.js
- packages/templates/clients/websocket/python/components/ClientClass.js
- packages/templates/clients/websocket/python/test/components/QueryParamsArgumentsDocs.test.js
- packages/templates/clients/websocket/python/template/client.py.js
- packages/templates/clients/websocket/python/test/components/Requires.test.js
packages/templates/clients/websocket/java/quarkus/components/ClientConnector.js
Outdated
Show resolved
Hide resolved
packages/templates/clients/websocket/java/quarkus/components/ClientFields.js
Outdated
Show resolved
Hide resolved
packages/templates/clients/websocket/java/quarkus/components/Constructor.js
Outdated
Show resolved
Hide resolved
|
Adi-204
left a comment
Adi-204
left a comment
There was a problem hiding this comment.
@SushanthMusham Tests are still failing, and I noticed that in the PR snapshot (QueryParamsVariables.test.js.snap) everything seems to have been incorrectly removed.
Also, as mentioned in the previous review, please make sure to always run npm run test from the root directory and ensure all tests pass before pushing changes. This will help avoid CI failures.
4 participants
Description
This PR refactors the
getQueryParamshelper function to return query parameters for all available channels, rather than just the first one. This is a foundational change to support multi-channel WebSocket/Kafka generations.Changes:
getQueryParamsin@asyncapi/generator-helpersto return an object keyed by channel names.@asyncapi/generator-react-sdk(QueryParamsVariables component)core-template-client-websocket-pythoncore-template-client-websocket-java-quarkusRelated Issue(s)
Fixes #1973
Checklist
npm test.Summary by CodeRabbit
Refactor
Templates
Tests
Documentation