feat: automatic routing of message for slack example using discriminators #1814
Conversation
🦋 Changeset detectedLatest commit: 16cf6f6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughAdds operation-discriminator-based message routing for WebSocket clients: new helpers to extract discriminators, new components to register and initialize per-operation handlers, updated Python onMessage logic to route to operation-specific handlers with fallback, plus tests, fixtures, example, and docs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. 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 |
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.
|
Adi-204
requested review from
asyncapi-bot-eve,
derberg and
magicmatatjahu
as code owners
![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 (8)
packages/templates/clients/kafka/java/quarkus/.ageneratorrc (1)
14-14: Verify if this formatting change was intentional.The removal of quotes from these YAML description strings is functionally harmless but appears unrelated to the PR's objective of implementing message routing for the Slack WebSocket example. This file is for the Kafka Java Quarkus template.
If this change was accidental (e.g., IDE auto-formatting), consider reverting it to keep the PR focused on its core functionality.
Also applies to: 18-18
packages/templates/clients/websocket/python/example-slack-with-routing.py (1)
29-45: Consider adding error handling to demonstrate best practices.While the registration logic is correct, adding try-except blocks around handler registration would make this example more robust and demonstrate error handling patterns for users.
🔎 Example with error handling
+try: client.register_on_hello_message_handler( handle_hello_message, discriminator_key="type", discriminator_value="hello" ) client.register_on_event_handler( handle_event_message, discriminator_key="type", discriminator_value="events_api" ) client.register_on_disconnect_message_handler( handle_disconnect_message, discriminator_key="type", discriminator_value="disconnect" ) +except Exception as e: + print(f"Failed to register handlers: {e}") + returnpackages/templates/clients/websocket/python/components/Constructor.js (1)
32-34: Consider improving template literal formatting for consistency.The multi-line initialization on Line 34 embeds a
\nin the string. For consistency with the rest of the template, consider using separateTextcomponents or line breaks in the template literal itself.🔎 Alternative formatting approach
- ${hasReceiveOperations ? 'self.receive_operation_handlers = {}\n self.receive_operation_discriminators = {}' : ''}` + ${hasReceiveOperations ? 'self.receive_operation_handlers = {}' : ''} + ${hasReceiveOperations ? 'self.receive_operation_discriminators = {}' : ''}`packages/templates/clients/websocket/python/components/RegisterReceiveOperations.js (3)
20-38: Add validation for discriminator parameters.The generated method accepts
discriminator_keyanddiscriminator_valuebut doesn't validate they're provided or non-empty. Add validation to prevent runtime errors during message routing.🔎 Suggested validation
if not callable(handler): raise TypeError(f"Handler for ${operationId} must be callable, got {type(handler).__name__}") + + if not discriminator_key or not isinstance(discriminator_key, str): + raise ValueError("discriminator_key must be a non-empty string") + + if not discriminator_value or not isinstance(discriminator_value, str): + raise ValueError("discriminator_value must be a non-empty string") # Register handler self.receive_operation_handlers["${operationId}"] = handler
19-19: Key attribute may be unnecessary.Based on learnings from this repository, keys aren't typically required when mapping over operations in generator components. Consider removing the
keyattribute unless React warnings are observed.🔎 Simplified version
- <Text indent={2} newLines={2} key={operationId}> + <Text indent={2} newLines={2}>Based on learnings, keys aren't required in generator components.
49-49: Consider reducing console output verbosity.The success message on Line 49 adds verbosity to the generated client. While helpful for debugging during development, consider making this debug-level logging or removing it for production clients to reduce console noise.
packages/templates/clients/websocket/python/components/AutoRouteMessages.js (2)
35-39: Consider using the registered error handler instead ofprint().The generated code uses
print()for error output, but the client class has ahandle_errormechanism (viaRegisterErrorHandler). Using it would provide consistent error handling and allow users to customize error behavior.Also, importing
tracebackinside the exception block is not idiomatic Python—imports should be at the module level.🔎 Proposed fix
try: handler(raw_message) return True except Exception as error: - print(f"Error in {operation_id} handler: {error}") - import traceback - traceback.print_exc() + self.handle_error(error) return TrueIf traceback detail is still desired, consider generating the
import tracebackat the top of the generated Python file/module rather than inline.
3-5: Enhance JSDoc with parameter and return information.Per the PR review guidelines, functions should have clear JSDoc comments including parameters, returns, and errors. This component takes no parameters and returns JSX, but documenting that explicitly improves maintainability.
🔎 Proposed enhancement
/** - * Generates generic message routing logic using user-registered discriminators + * Generates generic message routing logic using user-registered discriminators. + * + * This component renders a Python `_route_message` method that inspects incoming + * messages against registered discriminators and dispatches to the appropriate handler. + * + * @returns {JSX.Element} Text component containing the Python routing method */ export function AutoRouteMessages() {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
packages/components/test/components/__snapshots__/Connect.test.js.snapis excluded by!**/*.snappackages/components/test/components/__snapshots__/OnMessage.test.js.snapis excluded by!**/*.snappackages/templates/clients/websocket/python/test/components/__snapshots__/Constructor.test.js.snapis excluded by!**/*.snappackages/templates/clients/websocket/test/integration-test/__snapshots__/integration.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
packages/components/src/components/OnMessage.jspackages/templates/clients/kafka/java/quarkus/.ageneratorrcpackages/templates/clients/websocket/python/components/AutoRouteMessages.jspackages/templates/clients/websocket/python/components/ClientClass.jspackages/templates/clients/websocket/python/components/Constructor.jspackages/templates/clients/websocket/python/components/RegisterReceiveOperations.jspackages/templates/clients/websocket/python/example-slack-with-routing.pypackages/templates/clients/websocket/test/__fixtures__/asyncapi-slack-client.yml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: derberg
Repo: asyncapi/generator PR: 1551
File: apps/generator/docs/generator-template.md:45-73
Timestamp: 2025-05-12T14:23:48.919Z
Learning: AsyncAPI 3.0.0 specification introduces significant structural changes from 2.x:
1. Operations become top-level elements with action property (send/receive) and references to channels
2. Channels use 'address' for the topic path instead of having nested publish/subscribe operations
3. Messages are defined under a 'messages' container in channels
4. The specification decouples operations, channels and messages to improve reusability
5. Servers can use a 'host' property
📚 Learning: 2025-04-23T09:18:38.333Z
Learnt from: derberg
Repo: asyncapi/generator PR: 1512
File: packages/templates/clients/websocket/javascript/components/AvailableOperations.js:10-15
Timestamp: 2025-04-23T09:18:38.333Z
Learning: In the asyncapi/generator repository, keys aren't required when mapping over operations array in the AvailableOperations component, contrary to the typical React pattern.
Applied to files:
packages/templates/clients/websocket/python/components/RegisterReceiveOperations.js
🧬 Code graph analysis (1)
packages/templates/clients/websocket/python/components/RegisterReceiveOperations.js (4)
packages/templates/clients/websocket/python/components/ClientClass.js (1)
receiveOperations(19-19)apps/keeper/src/index.js (1)
operation(46-46)packages/helpers/src/utils.js (1)
toSnakeCase(70-76)packages/components/src/components/readme/OperationHeader.js (1)
summary(5-5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Acceptance tests for generated templates
- GitHub Check: Test generator as dependency with Node 24
- GitHub Check: Test NodeJS PR - ubuntu-latest
- GitHub Check: Test NodeJS PR - macos-latest
- GitHub Check: Test NodeJS PR - windows-latest
🔇 Additional comments (5)
packages/templates/clients/websocket/test/__fixtures__/asyncapi-slack-client.yml (1)
448-448: LGTM! Typo fix corrects JSON Schema keyword.The change from
conttoconstfixes a typo and correctly enforces a constant string value for the disconnect message type field, aligning with JSON Schema specification.packages/templates/clients/websocket/python/components/Constructor.js (1)
6-8: LGTM! Clean extension of Constructor API.The addition of the
receiveOperationsparameter and the corresponding validation check appropriately extend the constructor to support the new routing functionality while maintaining backward compatibility.packages/templates/clients/websocket/python/components/RegisterReceiveOperations.js (1)
8-11: LGTM! Clean guard clause.The early return for empty operations is good defensive programming and prevents unnecessary processing.
packages/templates/clients/websocket/python/components/ClientClass.js (1)
8-9: LGTM!The integration of automatic message routing follows the existing patterns established for
sendOperations. The new components are imported and wired correctly, withreceiveOperationscomputed viafilterByReceive()and passed to the relevant components.Also applies to: 19-19, 26-26, 48-49
packages/templates/clients/websocket/python/components/AutoRouteMessages.js (1)
34-34: Verify: returningTrueon exception may mask handler failures.When a handler is matched but throws an exception, the method returns
True(line 39), indicating the message was handled. This prevents fallback to the defaulthandle_messagepath, which may be intentional to avoid double-handling. However, it could also mask persistent handler failures.Consider whether this is the desired behavior, or if failed handlers should allow fallback (returning
False) so the message can still be processed by the generic handler.Also applies to: 39-39
packages/templates/clients/websocket/python/components/RegisterReceiveOperations.js
Show resolved
Hide resolved
packages/templates/clients/websocket/python/example-slack-with-routing.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/templates/clients/websocket/python/components/RegisterReceiveOperations.js (1)
34-36: Raise exception instead of silent failure.The generated code prints an error message and returns silently when a non-callable handler is provided. This makes debugging difficult during development. Raise a
TypeErrorinstead to ensure errors surface immediately.🔎 Proposed fix
if not callable(handler): - print("Handler must be callable") - return + raise TypeError(f"Handler for ${operationId} must be callable, got {type(handler).__name__}")
🧹 Nitpick comments (2)
packages/templates/clients/websocket/python/components/RegisterReceiveOperations.js (2)
19-47: Consider validating discriminator parameters.The generated registration method doesn't validate that
discriminator_keyanddiscriminator_valueare strings or non-empty. Invalid values could cause runtime errors during message routing when_route_message()attempts to access the discriminator field.🔎 Proposed validation
if not callable(handler): - print("Handler must be callable") - return + raise TypeError(f"Handler for ${operationId} must be callable, got {type(handler).__name__}") + + if not isinstance(discriminator_key, str) or not discriminator_key: + raise ValueError(f"discriminator_key must be a non-empty string, got {type(discriminator_key).__name__}") + + if not isinstance(discriminator_value, str) or not discriminator_value: + raise ValueError(f"discriminator_value must be a non-empty string, got {type(discriminator_value).__name__}")
38-45: Consider warning when overwriting handlers.The generated code silently overwrites any previously registered handler for the same operation. This could confuse users who accidentally call the registration method multiple times.
🔎 Proposed warning
+ # Warn if overwriting existing handler + if "${operationId}" in self.receive_operation_handlers: + print(f"Warning: Overwriting existing handler for operation: ${operationId}") + # Register handler self.receive_operation_handlers["${operationId}"] = handler
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/templates/clients/websocket/test/integration-test/__snapshots__/integration.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
packages/templates/clients/websocket/python/components/AutoRouteMessages.jspackages/templates/clients/websocket/python/components/RegisterReceiveOperations.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/templates/clients/websocket/python/components/AutoRouteMessages.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-23T09:18:38.333Z
Learnt from: derberg
Repo: asyncapi/generator PR: 1512
File: packages/templates/clients/websocket/javascript/components/AvailableOperations.js:10-15
Timestamp: 2025-04-23T09:18:38.333Z
Learning: In the asyncapi/generator repository, keys aren't required when mapping over operations array in the AvailableOperations component, contrary to the typical React pattern.
Applied to files:
packages/templates/clients/websocket/python/components/RegisterReceiveOperations.js
🧬 Code graph analysis (1)
packages/templates/clients/websocket/python/components/RegisterReceiveOperations.js (3)
packages/templates/clients/websocket/python/components/ClientClass.js (1)
receiveOperations(19-19)apps/keeper/src/index.js (1)
operation(46-46)packages/helpers/src/utils.js (1)
toSnakeCase(70-76)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test generator as dependency with Node 24
- GitHub Check: Acceptance tests for generated templates
- GitHub Check: Test NodeJS PR - ubuntu-latest
- GitHub Check: Test NodeJS PR - windows-latest
- GitHub Check: Test NodeJS PR - macos-latest
🔇 Additional comments (2)
packages/templates/clients/websocket/python/components/RegisterReceiveOperations.js (2)
1-11: LGTM: Clean component structure.The imports, JSDoc, and early return for empty operations are well-structured.
13-16: LGTM: Proper method name generation.The operationId extraction and snake_case conversion follow Python naming conventions correctly.
|
@asyncapi/bounty_team |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/templates/clients/websocket/test/__fixtures__/asyncapi-slack-client.yml (3)
275-292: Ensure the discriminator property is marked as required.The
typefield is now the discriminator for routinghellomessages, but it's not marked as required. If a message arrives without thetypefield, routing logic may fail or fall back unpredictably.📝 Suggested fix
hello: type: object discriminator: type + required: + - type properties: type: type: string const: hello
294-437: Ensure the discriminator property is marked as required.The
typefield is now the discriminator for routingeventmessages, but it's not marked as required. Messages without this field cannot be routed correctly.📝 Suggested fix
event: type: object discriminator: type additionalProperties: false + required: + - type properties:
447-462: Ensure the discriminator property is marked as required.The
typefield is now the discriminator for routingdisconnectmessages, but it's not marked as required. This creates a gap where messages without the discriminator field could bypass proper routing.📝 Suggested fix
disconnect: type: object discriminator: type + required: + - type properties: type: type: string const: disconnect
🤖 Fix all issues with AI agents
In
@packages/templates/clients/websocket/python/components/ReceiveOperationsDiscriminators.js:
- Around line 32-64: Add a JSDoc block above the exported
ReceiveOperationsDiscriminators function describing the function purpose, the
parameter (receiveOperations: array of operation objects with methods id(),
messages()), and the return type (JSX.Element|null); document that it returns
null when there are no receive operations and that it does not throw errors (or
note any possible error conditions if applicable), and include
@export/@returns/@param tags and a brief example or usage note per project
guidelines.
- Around line 3-30: Add a JSDoc block to the getDiscriminatorData function
describing parameters (message, operationId), return type (object|null) and
behavior, and remove the redundant check for !discriminator_key (since
discriminator is already validated earlier). Keep existing logic that extracts
payload, discriminator, properties and discriminatorProperty.const(), and ensure
the JSDoc documents that the function returns null on missing discriminator or
property and returns { key, value, operation_id } otherwise.
In
@packages/templates/clients/websocket/python/components/RegisterReceiveOperations.js:
- Around line 4-7: The file-level JSDoc above the RegisterReceiveOperations
component is missing parameter and return annotations; update the comment for
RegisterReceiveOperations (and the generated per-operation handler registration
methods) to include @param tags with types and descriptions for each expected
argument (e.g., operation name, handler callback, options/context), an @returns
tag describing the return value (e.g., unsubscribe function or void), and an
@throws/@throws or @error note for possible error conditions; ensure types use
JSDoc syntax (e.g., {string}, {Function}, {Object}) and mention any optional
parameters with brackets.
- Line 40: The generated Python template line containing "# Add discriminator
entry to the list if both key and value are provided" uses 3-space indentation
and should be changed to 4 spaces to match the surrounding code style; update
the template in RegisterReceiveOperations.js so that that comment and the
generated code line use a consistent 4-space indent (mirror the indentation used
around where the discriminator list is built) to avoid malformed Python output.
- Around line 48-49: Duplicate discriminators cause the same handler to run
multiple times; modify RegisterReceiveOperations.js where
receive_operation_discriminators is updated (the append of discriminator_entry)
to check for an existing discriminator (compare the unique key used by
AutoRouteMessages.js routing) and skip adding duplicates, or alternatively add a
single exit after a successful match in the routing function _route_message so
it breaks out of the loop once a handler has been invoked; update either
RegisterReceiveOperations.js to dedupe before appending or
AutoRouteMessages.js::_route_message to break after calling the matched handler.
In @packages/templates/clients/websocket/python/example-slack-with-routing.py:
- Around line 5-7: The example modifies sys.path and imports from a
test-specific module path (temp.snapshotTestResult.client_slack.client) which is
non-portable; replace the sys.path manipulation and the test-specific import
with the real generated client import (e.g., import SlackWebsocketAPIClient from
the package name used by codegen such as client_slack.client or
your_generated_client.client_slack.client) or add a clear comment explaining the
required directory layout and how to set PYTHONPATH; update references to the
SlackWebsocketAPIClient symbol and remove the temporary test path manipulation
so the example works for consumers without the test/ temp folder.
- Around line 22-23: Remove the unused helper function
handle_unrecognized_message from the example file: it isn’t referenced or
registerable by the generated client’s routing logic, so delete the entire
function definition to avoid confusion and dead code; search for the symbol
handle_unrecognized_message to locate and remove it and ensure no imports or
calls reference it afterward.
🧹 Nitpick comments (6)
packages/templates/clients/websocket/python/components/ReceiveOperationsDiscriminators.js (2)
57-62: Clarify indentation strategy for generated Python code.The template mixes the
indent={2}prop with hardcoded leading whitespace in the template string. This approach can be fragile and makes it unclear what the final indentation will be in the generated Python code.Consider consolidating the indentation approach—either rely solely on the
indentprop or clearly document why both mechanisms are needed (e.g., if this code block is meant to be nested within a Python class method).
64-64: Add newline at end of file.The file is missing a trailing newline, which is a common convention in most codebases and expected by many linters and version control systems.
packages/templates/clients/websocket/python/components/RegisterReceiveOperations.js (1)
28-35: Consider more Pythonic error handling.The generated code uses
print()and earlyreturnfor error conditions. While functional, more idiomatic Python would raise exceptions (TypeErrorfor non-callable handlers,ValueErrorfor invalid discriminator combinations). This would allow callers to catch and handle errors programmatically.However, if the simpler approach is intentional for easier debugging in generated client code, this can remain as-is.
♻️ Alternative: Pythonic error handling
if not callable(handler): - print("Handler must be callable") - return + raise TypeError("Handler must be callable") # Validate that either both discriminator_key and discriminator_value are provided or neither if (discriminator_key is not None and discriminator_value is None) or (discriminator_key is None and discriminator_value is not None): - print("Error: Both discriminator_key and discriminator_value must be provided together") - return + raise ValueError("Both discriminator_key and discriminator_value must be provided together")packages/templates/clients/websocket/python/example-slack-with-routing.py (3)
37-38: Consider a more reasonable duration or making it configurable.The 1000-second sleep (approximately 16.7 minutes) seems arbitrary for an example. Consider either:
- Using a shorter, more reasonable duration for demonstration purposes (e.g., 60 seconds)
- Making it configurable via a command-line argument or constant
- Adding a comment explaining why this specific duration is used
💡 Suggested improvement
+# Duration to keep the connection alive (in seconds) +KEEP_ALIVE_DURATION = 60 # Adjust as needed + def main(): client = SlackWebsocketAPIClient() client.register_on_hello_message_handler(handle_hello_message) client.register_on_event_handler(handle_event_message) client.register_on_disconnect_message_handler(handle_disconnect_message) client.connect() # Keep program alive for a while to allow message processing - time.sleep(1000) + print(f"Connection established. Keeping alive for {KEEP_ALIVE_DURATION} seconds...") + time.sleep(KEEP_ALIVE_DURATION)
26-38: Add error handling and resource cleanup.The example lacks error handling for connection failures and doesn't properly clean up resources. For a production-ready example, consider:
- Wrapping the connection logic in a try-except block
- Adding a finally block or context manager to ensure cleanup
- Handling keyboard interrupts gracefully
🛡️ Proposed improvement with error handling and cleanup
def main(): client = SlackWebsocketAPIClient() client.register_on_hello_message_handler(handle_hello_message) client.register_on_event_handler(handle_event_message) client.register_on_disconnect_message_handler(handle_disconnect_message) - client.connect() - - # Keep program alive for a while to allow message processing - time.sleep(1000) + try: + client.connect() + print("Connection established. Press Ctrl+C to stop...") + + # Keep program alive for a while to allow message processing + time.sleep(1000) + except KeyboardInterrupt: + print("\nShutting down gracefully...") + except Exception as e: + print(f"Error: {e}") + finally: + # Cleanup: disconnect if the client has a disconnect method + if hasattr(client, 'disconnect'): + client.disconnect()
41-41: Use the standard Pythonif __name__ == "__main__":guard.The example directly calls
main()without the standardif __name__ == "__main__":guard. This means the code will execute even when the file is imported as a module, which is generally undesirable.🐍 Standard Python entry point pattern
-main() +if __name__ == "__main__": + main()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/templates/clients/websocket/test/integration-test/__snapshots__/integration.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
packages/templates/clients/websocket/python/components/AutoRouteMessages.jspackages/templates/clients/websocket/python/components/Constructor.jspackages/templates/clients/websocket/python/components/ReceiveOperationsDiscriminators.jspackages/templates/clients/websocket/python/components/RegisterReceiveOperations.jspackages/templates/clients/websocket/python/example-slack-with-routing.pypackages/templates/clients/websocket/test/__fixtures__/asyncapi-slack-client.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/templates/clients/websocket/python/components/AutoRouteMessages.js
- packages/templates/clients/websocket/python/components/Constructor.js
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/pr-review-checklist.md)
Write clear and consistent JSDoc comments for functions, including parameter types, return values, and error conditions
Files:
packages/templates/clients/websocket/python/components/RegisterReceiveOperations.jspackages/templates/clients/websocket/python/components/ReceiveOperationsDiscriminators.js
🧠 Learnings (3)
📚 Learning: 2025-04-23T09:18:38.333Z
Learnt from: derberg
Repo: asyncapi/generator PR: 1512
File: packages/templates/clients/websocket/javascript/components/AvailableOperations.js:10-15
Timestamp: 2025-04-23T09:18:38.333Z
Learning: In the asyncapi/generator repository, keys aren't required when mapping over operations array in the AvailableOperations component, contrary to the typical React pattern.
Applied to files:
packages/templates/clients/websocket/python/components/RegisterReceiveOperations.jspackages/templates/clients/websocket/python/components/ReceiveOperationsDiscriminators.js
📚 Learning: 2025-05-12T14:23:48.919Z
Learnt from: derberg
Repo: asyncapi/generator PR: 1551
File: apps/generator/docs/generator-template.md:45-73
Timestamp: 2025-05-12T14:23:48.919Z
Learning: AsyncAPI 3.0.0 specification introduces significant structural changes from 2.x:
1. Operations become top-level elements with action property (send/receive) and references to channels
2. Channels use 'address' for the topic path instead of having nested publish/subscribe operations
3. Messages are defined under a 'messages' container in channels
4. The specification decouples operations, channels and messages to improve reusability
5. Servers can use a 'host' property
Applied to files:
packages/templates/clients/websocket/test/__fixtures__/asyncapi-slack-client.yml
📚 Learning: 2025-05-12T14:23:48.919Z
Learnt from: derberg
Repo: asyncapi/generator PR: 1551
File: apps/generator/docs/generator-template.md:45-73
Timestamp: 2025-05-12T14:23:48.919Z
Learning: AsyncAPI 3.0.0 specification introduces significant structural changes from 2.x:
1. Operations become top-level elements with references to channels
2. Channels use 'address' for the topic path instead of having nested publish/subscribe
3. Messages are defined under a 'messages' container in channels
4. Servers can use a 'host' property
Applied to files:
packages/templates/clients/websocket/test/__fixtures__/asyncapi-slack-client.yml
🧬 Code graph analysis (2)
packages/templates/clients/websocket/python/components/RegisterReceiveOperations.js (3)
packages/templates/clients/websocket/python/components/ClientClass.js (1)
receiveOperations(19-19)apps/keeper/src/index.js (1)
operation(46-46)packages/helpers/src/utils.js (1)
toSnakeCase(70-76)
packages/templates/clients/websocket/python/components/ReceiveOperationsDiscriminators.js (1)
apps/keeper/src/index.js (1)
operation(46-46)
🔇 Additional comments (1)
packages/templates/clients/websocket/test/__fixtures__/asyncapi-slack-client.yml (1)
277-277: No changes required.The discriminator format is correct for AsyncAPI 3.0. According to the AsyncAPI 3.0 specification, discriminator is a string field representing the property name used for polymorphism, not an object format. The current usage (
discriminator: type) properly aligns with the AsyncAPI 3.0 specification.Likely an incorrect or invalid review comment.
packages/templates/clients/websocket/python/components/ReceiveOperationsDiscriminators.js
Outdated
Show resolved
Hide resolved
packages/templates/clients/websocket/python/components/ReceiveOperationsDiscriminators.js
Outdated
Show resolved
Hide resolved
packages/templates/clients/websocket/python/components/RegisterReceiveOperations.js
Show resolved
Hide resolved
packages/templates/clients/websocket/python/components/RegisterReceiveOperations.js
Outdated
Show resolved
Hide resolved
packages/templates/clients/websocket/python/components/RegisterReceiveOperations.js
Outdated
Show resolved
Hide resolved
packages/templates/clients/websocket/python/example-slack-with-routing.py
Show resolved
Hide resolved
packages/templates/clients/websocket/python/example-slack-with-routing.py
Show resolved
Hide resolved
|
@derberg I’ve implemented the changes we discussed during the meeting. While working on them, I realized that the initial solution assumed only a single message per receive operation. Since a receive operation can contain multiple messages, I had to refactor the approach more extensively. The updated design now uses self.receive_operation_discriminators = [
{ "key": "type", "value": "hello", "operation_id": "onHelloMessage" },
{ "key": "type", "value": "events_api", "operation_id": "onEvent" },
{ "key": "type", "value": "disconnect", "operation_id": "onDisconnectMessage" }
]With this implementation, the routing logic correctly supports multiple messages within the same receive operation, rather than assuming a one-to-one relationship. Limitation of the current approachAs discussed, by default we derive:
For non-default cases, the user must explicitly provide both Why we can’t support only one of
|
derberg
left a comment
derberg
left a comment
There was a problem hiding this comment.
reremember to add changeset, we need to release components library and generator
use #1814 (comment) as input for examplantion on why solution is designed at the moment this way. Mention that at the moment asyncapi 3.0 had descriminator value as string only
packages/templates/clients/websocket/python/example-slack-with-routing.py
Show resolved
Hide resolved
packages/templates/clients/websocket/python/components/ReceiveOperationsDiscriminators.js
Outdated
Show resolved
Hide resolved
packages/templates/clients/websocket/python/components/AutoRouteMessages.js
Outdated
Show resolved
Hide resolved
packages/templates/clients/websocket/python/components/ReceiveOperationsDiscriminators.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @packages/helpers/src/discriminators.js:
- Around line 8-14: The function getMessageDiscriminatorData calls
message.payload() and immediately uses payload.discriminator(), which will throw
if payload is null/undefined; add a defensive guard after calling
message.payload() to return null (or handle appropriately) when payload is falsy
before calling payload.discriminator(), and then proceed to check discriminator
as currently done—update references in getMessageDiscriminatorData to validate
payload first and only call payload.discriminator() when payload is non-null.
- Around line 37-42: The JSDoc for the function that collects discriminators is
wrong: update the @returns annotation in the JSDoc of the function in
packages/helpers/src/discriminators.js (the function that returns the
discriminator metadata array, referenced where it returns on line ~60) from
`{string} JSON stringified array` to the correct type such as `{Array<object>}`
(or `{Array}`) and change the description to “array of discriminator metadata”
so the doc accurately reflects that the function returns an array (serialization
is done by the consumer `OperationsDiscriminators`).
In @packages/templates/clients/websocket/python/README.md:
- Around line 72-76: The numbered step in the auto-routing Slack client README
is misleading—update the text so it clearly continues the prior Slack client
steps instead of restarting at "1."; either change the lone "1." to "4." (or the
appropriate next number) or add a clarifying phrase like "Continue from the
previous steps above, then:" and keep the two exceptions (use
example-slack-with-routing.py and that it auto-routes events) and the remaining
instruction to start the example that uses the generated client with
auto-routing; ensure references to example-slack.py and
example-slack-with-routing.py remain unchanged.
🧹 Nitpick comments (6)
packages/helpers/src/discriminators.js (1)
46-58: Consider defensive check foroperation.messages().If an operation has no messages defined or
messages()returns null, calling.all()on line 48 would throw. This edge case may occur with malformed specs.Proposed defensive approach
operations.forEach((operation) => { const operationId = operation.id(); - const messages = operation.messages().all(); + const messagesCollection = operation.messages(); + if (!messagesCollection) { + return; + } + const messages = messagesCollection.all(); messages .filter(message => message.hasPayload())packages/components/test/components/OperationsDiscriminators.test.js (2)
13-18: Add error handling for parse failures.If parsing fails,
parseResult.documentwill be undefined, causing subsequent lines to throw. Consider adding validation.Proposed fix
beforeAll(async () => { const parseResult = await fromFile(parser, asyncapi_websocket_query).parse(); + if (!parseResult.document) { + throw new Error(`Failed to parse AsyncAPI document: ${JSON.stringify(parseResult.diagnostics)}`); + } parsedAsyncAPIDocument = parseResult.document; const operations = parsedAsyncAPIDocument.operations(); receiveOperations = operations.filterByReceive(); });
20-24: Consider adding edge case tests.The current test covers the happy path for Python. Consider adding tests for:
- Empty operations array (should return null)
- Unsupported language (should return null)
- Operations with no discriminators
These would improve confidence in the component's defensive behavior.
Example additional tests
test('returns null for empty operations', () => { const result = render(<OperationsDiscriminators language='python' operations={[]} />); expect(result.trim()).toBe(''); }); test('returns null for unsupported language', () => { const result = render(<OperationsDiscriminators language='unsupported' operations={receiveOperations} />); expect(result.trim()).toBe(''); });packages/helpers/test/discriminators.test.js (2)
45-57: Consider clarifying or consolidating redundant test cases.Both tests (lines 45-50 and 52-57) use the same
noExamplesmessage and expectnull. The distinction between "payload is not an object schema" and "existing message without discriminator" is unclear when using the same message. Consider either:
- Using different messages that clearly demonstrate each scenario, or
- Consolidating into a single test with a more descriptive name.
68-83: Good structural validation, consider adding exact count assertion.The test validates array structure and property types well. For more deterministic verification, consider asserting the exact expected count of discriminators rather than just
length > 0.packages/templates/clients/websocket/python/components/Constructor.js (1)
6-6: Add JSDoc documentation for the function and its parameters.Per coding guidelines, functions should have clear JSDoc comments including parameter types, return values, and descriptions. The
receiveOperationsparameter is new and undocumented.📝 Suggested JSDoc
+/** + * Generates the Python WebSocket client constructor initialization code. + * + * @param {object} props - Component props. + * @param {string} props.serverUrl - The WebSocket server URL. + * @param {Map|null} props.query - Query parameters map. + * @param {Array} props.receiveOperations - Receive operations from AsyncAPI document. + * @returns {React.Element} Rendered constructor initialization code. + */ export function Constructor({ serverUrl, query, receiveOperations }) {As per coding guidelines, write clear and consistent JSDoc comments for functions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
packages/components/test/components/__snapshots__/AvailableOperations.test.js.snapis excluded by!**/*.snappackages/components/test/components/__snapshots__/Connect.test.js.snapis excluded by!**/*.snappackages/components/test/components/__snapshots__/OnMessage.test.js.snapis excluded by!**/*.snappackages/components/test/components/__snapshots__/OperationsDiscriminators.test.js.snapis excluded by!**/*.snappackages/templates/clients/websocket/python/test/components/__snapshots__/Constructor.test.js.snapis excluded by!**/*.snappackages/templates/clients/websocket/test/integration-test/__snapshots__/integration.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
.changeset/slack-auto-routing.mdpackages/components/src/components/OnMessage.jspackages/components/src/components/OperationsDiscriminators.jspackages/components/src/index.jspackages/components/test/components/OperationsDiscriminators.test.jspackages/helpers/src/discriminators.jspackages/helpers/src/index.jspackages/helpers/test/__fixtures__/asyncapi-websocket-query.ymlpackages/helpers/test/discriminators.test.jspackages/templates/clients/websocket/python/README.mdpackages/templates/clients/websocket/python/components/ClientClass.jspackages/templates/clients/websocket/python/components/Constructor.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/components/src/components/OnMessage.js
- packages/templates/clients/websocket/python/components/ClientClass.js
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/pr-review-checklist.md)
Write clear and consistent JSDoc comments for functions, including parameter types, return values, and error conditions
Files:
packages/components/src/index.jspackages/components/test/components/OperationsDiscriminators.test.jspackages/helpers/src/index.jspackages/helpers/test/discriminators.test.jspackages/helpers/src/discriminators.jspackages/components/src/components/OperationsDiscriminators.jspackages/templates/clients/websocket/python/components/Constructor.js
.changeset/*.md
📄 CodeRabbit inference engine (Development.md)
Create changeset markdown files in the .changeset directory to document release changes with package name and version type (major/minor/patch)
Files:
.changeset/slack-auto-routing.md
🧠 Learnings (5)
📓 Common learnings
Learnt from: derberg
Repo: asyncapi/generator PR: 1551
File: apps/generator/docs/generator-template.md:45-73
Timestamp: 2025-05-12T14:23:48.919Z
Learning: AsyncAPI 3.0.0 specification introduces significant structural changes from 2.x:
1. Operations become top-level elements with action property (send/receive) and references to channels
2. Channels use 'address' for the topic path instead of having nested publish/subscribe operations
3. Messages are defined under a 'messages' container in channels
4. The specification decouples operations, channels and messages to improve reusability
5. Servers can use a 'host' property
📚 Learning: 2026-01-05T09:57:44.281Z
Learnt from: CR
Repo: asyncapi/generator PR: 0
File: Development.md:0-0
Timestamp: 2026-01-05T09:57:44.281Z
Learning: Applies to apps/generator/test/**/*.test.{js,ts} : Create new test files in the appropriate directory under apps/generator/test/ and follow existing test patterns
Applied to files:
packages/components/test/components/OperationsDiscriminators.test.js
📚 Learning: 2025-05-12T14:23:48.919Z
Learnt from: derberg
Repo: asyncapi/generator PR: 1551
File: apps/generator/docs/generator-template.md:45-73
Timestamp: 2025-05-12T14:23:48.919Z
Learning: AsyncAPI 3.0.0 specification introduces significant structural changes from 2.x:
1. Operations become top-level elements with action property (send/receive) and references to channels
2. Channels use 'address' for the topic path instead of having nested publish/subscribe operations
3. Messages are defined under a 'messages' container in channels
4. The specification decouples operations, channels and messages to improve reusability
5. Servers can use a 'host' property
Applied to files:
.changeset/slack-auto-routing.mdpackages/helpers/test/__fixtures__/asyncapi-websocket-query.yml
📚 Learning: 2025-05-12T14:23:48.919Z
Learnt from: derberg
Repo: asyncapi/generator PR: 1551
File: apps/generator/docs/generator-template.md:45-73
Timestamp: 2025-05-12T14:23:48.919Z
Learning: AsyncAPI 3.0.0 specification introduces significant structural changes from 2.x:
1. Operations become top-level elements with references to channels
2. Channels use 'address' for the topic path instead of having nested publish/subscribe
3. Messages are defined under a 'messages' container in channels
4. Servers can use a 'host' property
Applied to files:
.changeset/slack-auto-routing.mdpackages/helpers/test/__fixtures__/asyncapi-websocket-query.yml
📚 Learning: 2025-04-23T09:18:38.333Z
Learnt from: derberg
Repo: asyncapi/generator PR: 1512
File: packages/templates/clients/websocket/javascript/components/AvailableOperations.js:10-15
Timestamp: 2025-04-23T09:18:38.333Z
Learning: In the asyncapi/generator repository, keys aren't required when mapping over operations array in the AvailableOperations component, contrary to the typical React pattern.
Applied to files:
packages/components/src/components/OperationsDiscriminators.jspackages/templates/clients/websocket/python/components/Constructor.js
🧬 Code graph analysis (6)
packages/components/test/components/OperationsDiscriminators.test.js (3)
packages/helpers/test/discriminators.test.js (1)
parser(5-5)apps/react-sdk/src/renderer/renderer.ts (1)
render(63-77)packages/components/src/components/OperationsDiscriminators.js (1)
OperationsDiscriminators(22-42)
packages/helpers/src/index.js (2)
packages/helpers/test/discriminators.test.js (2)
require(2-2)require(3-3)packages/helpers/test/testing.test.js (2)
require(1-1)require(3-3)
packages/helpers/test/discriminators.test.js (1)
packages/helpers/src/discriminators.js (2)
getMessageDiscriminatorData(8-35)getMessageDiscriminatorsFromOperations(43-61)
packages/helpers/src/discriminators.js (5)
packages/templates/clients/websocket/javascript/example.js (1)
message(26-26)packages/templates/clients/websocket/python/components/ClientClass.js (1)
operations(16-16)packages/components/src/components/OperationsDiscriminators.js (1)
operationDiscriminators(33-33)apps/keeper/src/index.js (1)
operation(46-46)packages/helpers/test/discriminators.test.js (1)
messages(10-10)
packages/components/src/components/OperationsDiscriminators.js (2)
packages/templates/clients/websocket/python/components/ClientClass.js (1)
operations(16-16)packages/helpers/src/discriminators.js (2)
operationDiscriminators(44-44)getMessageDiscriminatorsFromOperations(43-61)
packages/templates/clients/websocket/python/components/Constructor.js (3)
packages/templates/clients/websocket/javascript/components/Constructor.js (1)
Constructor(3-27)packages/templates/clients/websocket/python/components/ClientClass.js (1)
receiveOperations(18-18)packages/components/src/components/OperationsDiscriminators.js (1)
OperationsDiscriminators(22-42)
🔇 Additional comments (10)
.changeset/slack-auto-routing.md (1)
1-10: LGTM!The changeset follows the expected format with proper frontmatter specifying package names and version types. Minor version bumps are appropriate for these non-breaking feature additions. The description clearly documents the new components and helpers being introduced.
packages/helpers/test/__fixtures__/asyncapi-websocket-query.yml (2)
77-106: Good test coverage for discriminator edge cases.The three new message definitions provide excellent coverage:
MarketUpdateMessage: valid discriminator withconstvalue (happy path)MessageWithoutDiscriminator: tests the null return when discriminator is absentMessageWithDiscriminatorNoConst: tests the null return whenconstis missingThis aligns well with the AsyncAPI 3.0.0 structure. Based on learnings, the decoupled operations/channels/messages pattern is correctly followed.
196-205: LGTM!The
receiveMarketUpdateoperation correctly references all three test messages, enabling comprehensive testing of the discriminator extraction across mixed message types within a single operation.packages/components/src/index.js (1)
21-21: LGTM!The new export follows the established pattern in this file and correctly exposes the
OperationsDiscriminatorscomponent from the public API surface.packages/helpers/src/index.js (1)
3-4: LGTM!The new discriminator utilities are properly imported and exported, following the existing patterns in this module.
Also applies to: 30-32
packages/helpers/test/discriminators.test.js (1)
1-101: LGTM overall - solid test coverage for discriminator utilities.The tests cover the main success paths and edge cases (missing discriminator, no const, non-object payload, empty operations). The structure validation ensures type safety of returned objects.
packages/templates/clients/websocket/python/README.md (1)
87-87: Good documentation of discriminator constraints.This clearly communicates the requirement for both
discriminator_keyanddiscriminator_valuewhen using non-default cases, aligning with the design rationale discussed in the PR comments about avoiding routing ambiguity.packages/templates/clients/websocket/python/components/Constructor.js (1)
30-37: LGTM - Proper conditional initialization and component usage.The guard on line 31 correctly initializes
receive_operation_handlersonly whenreceiveOperationsexists and is non-empty. TheOperationsDiscriminatorscomponent will handle its own null guards internally.packages/components/src/components/OperationsDiscriminators.js (2)
22-41: LGTM - Well-structured component with proper guards.The component follows good practices:
- Validates inputs before processing
- Returns
nullfor invalid states (empty operations, unsupported language)- Separates language-specific logic into a lookup table for extensibility
- Good JSDoc documentation
8-11: Add JSDoc documentation for the python initializer function.The
pythonfunction withinlanguageSpecificInitializers(lines 9-10) lacks JSDoc documentation. Per the coding guidelines, add parameter types, return value, and description. Additionally, the concern about JSON escaping incompatibility is not valid—JSON.stringifyoutput is valid Python dictionary syntax, and differences in how JavaScript and Python escape non-ASCII characters produce semantically equivalent results within JSON format.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @packages/components/src/components/OnMessage.js:
- Around line 49-51: The code assumes parsed_message is a dict before calling
parsed_message.get(key), which will raise if parsed_message is a
list/string/number or parsing failed; update the condition in the discriminator
check to first ensure parsed_message is a dict (e.g., isinstance(parsed_message,
dict) or typeof check depending on runtime) and only then check key and
parsed_message.get(key) == value, so you safely lookup and then resolve handler
from receive_operation_handlers using operation_id.
- Around line 34-61: The on_message handler has inconsistent indentation (mixed
6-space and 2-space) which will raise Python IndentationError; edit the
on_message function so all nested blocks (try/except, for loop, if checks, and
inner try/except) use consistent 2-space indentation throughout, ensuring the
bodies of on_message, the parsing try/except, the for loop over
self.receive_operation_discriminators, the if key check, the handler invocation
and its except, and the final if not handled: self.handle_message(message) are
all aligned; verify identifiers receive_operation_discriminators,
receive_operation_handlers, operation_id, handler, and handle_message remain
unchanged.
In @packages/helpers/src/discriminators.js:
- Around line 37-42: The JSDoc for the function in
packages/helpers/src/discriminators.js incorrectly documents the return type as
a string; update the @returns tag to reflect that the function returns an array
(e.g., @returns {Array<object>} or @returns {Array} with a short description) so
it matches the actual return value and how OperationsDiscriminators.js consumes
it; adjust the description text accordingly to say it returns an array of
discriminator metadata instead of a JSON stringified array.
🧹 Nitpick comments (9)
packages/templates/clients/websocket/python/README.md (1)
69-87: Documentation looks good, minor formatting issue with list numbering.The new auto-routing section provides clear guidance. However, the numbered list at lines 76 and 87 starts fresh at "1." when it should arguably continue from the previous section's numbering (step 4 would be the next logical step after step 3 in the Slack client section), or you should clarify this is a separate sequence.
Also, line 87 is listed as step "1." but reads more like a general note than a step. Consider reformatting it as a blockquote or note:
📝 Suggested formatting
-1. By default, `discriminator_key` is derived from the discriminator field, and `discriminator_value` from the corresponding `const` in the AsyncAPI document. For non-default cases, users must provide both `discriminator_key` and `discriminator_value` explicitly in register_handlers. Partial inputs are not supported to avoid ambiguity in message routing. +> **Note**: By default, `discriminator_key` is derived from the discriminator field, and `discriminator_value` from the corresponding `const` in the AsyncAPI document. For non-default cases, users must provide both `discriminator_key` and `discriminator_value` explicitly in register_handlers. Partial inputs are not supported to avoid ambiguity in message routing.packages/helpers/src/discriminators.js (1)
8-14: Consider adding defensive null check for payload.While the caller filters with
message.hasPayload()before invoking this function, adding a defensive check for the payload would make this function more robust when called from other contexts.♻️ Optional defensive enhancement
const getMessageDiscriminatorData = (message, operationId) => { const payload = message.payload(); + if (!payload) { + return null; + } const discriminator = payload.discriminator(); if (!discriminator) { return null; }packages/components/src/components/OnMessage.js (1)
52-57: Consider breaking after first successful handler match.The loop continues iterating even after a handler successfully processes the message. If multiple discriminators match, multiple handlers could be invoked, which may not be the intended behavior for message routing.
♻️ Suggested fix to break after first match
if handler: try: handler(message) handled = True + break except Exception as error: print(f"Error in {operation_id} handler: {error}")Could you confirm the intended behavior? Should a message be routed to only the first matching handler, or should all matching handlers be invoked?
packages/components/src/components/OperationsDiscriminators.js (1)
13-21: JSDoc is clear but could note null return conditions.Consider documenting that the function returns
nullwhen the language is unsupported or when operations is empty. This helps consumers understand the component's behavior./** * Generates language-specific initialization code * for receive operation discriminators. * * @param {object} props - Component props. * @param {string} props.language - Target language (e.g. "python"). * @param {Array} props.operations - Receive operations from AsyncAPI document. - * @returns {React.Element|null} Rendered initialization code or null. + * @returns {React.Element|null} Rendered initialization code, or null if + * operations is empty/missing or if the language is not supported. */packages/templates/clients/websocket/python/components/Constructor.js (2)
6-6: Add JSDoc documentation for the Constructor function.Per coding guidelines, functions should have JSDoc comments including parameter types, return values, and descriptions.
📝 Suggested JSDoc
+/** + * Generates the Python __init__ method for the WebSocket client class. + * + * @param {object} props - Component props. + * @param {string} props.serverUrl - The WebSocket server URL. + * @param {Map|null} props.query - Query parameters from channel bindings. + * @param {Array} [props.receiveOperations] - Receive operations for message routing. + * @returns {React.Element} Rendered Python constructor code. + */ export function Constructor({ serverUrl, query, receiveOperations }) {
30-31: Consider using optional chaining for cleaner conditional.The condition on line 31 can be simplified using optional chaining.
♻️ Suggested simplification
self._stop_event = threading.Event() ${ query ? 'params = {}' : ''} - ${ (receiveOperations && receiveOperations.length > 0) ? 'self.receive_operation_handlers = {}' : ''}` + ${ receiveOperations?.length > 0 ? 'self.receive_operation_handlers = {}' : ''}`packages/components/test/components/OperationsDiscriminators.test.js (1)
20-24: Consider adding edge case tests.The snapshot test covers the happy path for Python, but additional tests would improve confidence:
🧪 Suggested additional test cases
test('returns empty when language is unsupported', () => { const result = render(<OperationsDiscriminators language='unsupported' operations={receiveOperations} />); expect(result.trim()).toBe(''); }); test('returns empty when operations array is empty', () => { const result = render(<OperationsDiscriminators language='python' operations={[]} />); expect(result.trim()).toBe(''); }); test('returns empty when operations is undefined', () => { const result = render(<OperationsDiscriminators language='python' operations={undefined} />); expect(result.trim()).toBe(''); });packages/helpers/test/discriminators.test.js (2)
6-6: Consider using camelCase for variable naming.The variable
asyncapi_v3_pathuses snake_case, which is inconsistent with JavaScript naming conventions. Consider renaming toasyncapiV3Path.Suggested fix
-const asyncapi_v3_path = path.resolve(__dirname, './__fixtures__/asyncapi-websocket-query.yml'); +const asyncapiV3Path = path.resolve(__dirname, './__fixtures__/asyncapi-websocket-query.yml');Also update usages on lines 14 and 64.
13-18: Add parse error handling for more robust tests.The
parseResultmay contain errors if parsing fails. Consider adding validation to make test failures more informative.Suggested fix
beforeAll(async () => { const parseResult = await fromFile(parser, asyncapi_v3_path).parse(); + if (parseResult.diagnostics.some(d => d.severity === 0)) { + throw new Error(`Failed to parse AsyncAPI document: ${JSON.stringify(parseResult.diagnostics)}`); + } parsedAsyncAPIDocument = parseResult.document; channel = parsedAsyncAPIDocument.channels().get('marketDataV1'); messages = channel.messages(); });Apply the same pattern to the second
beforeAllblock (lines 63-66).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
packages/components/test/components/__snapshots__/AvailableOperations.test.js.snapis excluded by!**/*.snappackages/components/test/components/__snapshots__/Connect.test.js.snapis excluded by!**/*.snappackages/components/test/components/__snapshots__/OnMessage.test.js.snapis excluded by!**/*.snappackages/components/test/components/__snapshots__/OperationsDiscriminators.test.js.snapis excluded by!**/*.snappackages/templates/clients/websocket/python/test/components/__snapshots__/Constructor.test.js.snapis excluded by!**/*.snappackages/templates/clients/websocket/test/integration-test/__snapshots__/integration.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
.changeset/slack-auto-routing.mdpackages/components/src/components/OnMessage.jspackages/components/src/components/OperationsDiscriminators.jspackages/components/src/index.jspackages/components/test/components/OperationsDiscriminators.test.jspackages/helpers/src/discriminators.jspackages/helpers/src/index.jspackages/helpers/test/__fixtures__/asyncapi-websocket-query.ymlpackages/helpers/test/discriminators.test.jspackages/templates/clients/websocket/python/README.mdpackages/templates/clients/websocket/python/components/ClientClass.jspackages/templates/clients/websocket/python/components/Constructor.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/templates/clients/websocket/python/components/ClientClass.js
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/pr-review-checklist.md)
Write clear and consistent JSDoc comments for functions, including parameter types, return values, and error conditions
Files:
packages/components/src/components/OnMessage.jspackages/helpers/test/discriminators.test.jspackages/templates/clients/websocket/python/components/Constructor.jspackages/helpers/src/discriminators.jspackages/components/src/index.jspackages/helpers/src/index.jspackages/components/src/components/OperationsDiscriminators.jspackages/components/test/components/OperationsDiscriminators.test.js
.changeset/*.md
📄 CodeRabbit inference engine (Development.md)
Create changeset markdown files in the .changeset directory to document release changes with package name and version type (major/minor/patch)
Files:
.changeset/slack-auto-routing.md
🧠 Learnings (4)
📚 Learning: 2026-01-05T09:57:44.281Z
Learnt from: CR
Repo: asyncapi/generator PR: 0
File: Development.md:0-0
Timestamp: 2026-01-05T09:57:44.281Z
Learning: Applies to apps/generator/test/**/*.test.{js,ts} : Create new test files in the appropriate directory under apps/generator/test/ and follow existing test patterns
Applied to files:
packages/helpers/test/discriminators.test.js
📚 Learning: 2025-04-23T09:18:38.333Z
Learnt from: derberg
Repo: asyncapi/generator PR: 1512
File: packages/templates/clients/websocket/javascript/components/AvailableOperations.js:10-15
Timestamp: 2025-04-23T09:18:38.333Z
Learning: In the asyncapi/generator repository, keys aren't required when mapping over operations array in the AvailableOperations component, contrary to the typical React pattern.
Applied to files:
packages/templates/clients/websocket/python/components/Constructor.jspackages/components/src/components/OperationsDiscriminators.js
📚 Learning: 2025-05-12T14:23:48.919Z
Learnt from: derberg
Repo: asyncapi/generator PR: 1551
File: apps/generator/docs/generator-template.md:45-73
Timestamp: 2025-05-12T14:23:48.919Z
Learning: AsyncAPI 3.0.0 specification introduces significant structural changes from 2.x:
1. Operations become top-level elements with action property (send/receive) and references to channels
2. Channels use 'address' for the topic path instead of having nested publish/subscribe operations
3. Messages are defined under a 'messages' container in channels
4. The specification decouples operations, channels and messages to improve reusability
5. Servers can use a 'host' property
Applied to files:
.changeset/slack-auto-routing.mdpackages/helpers/test/__fixtures__/asyncapi-websocket-query.yml
📚 Learning: 2025-05-12T14:23:48.919Z
Learnt from: derberg
Repo: asyncapi/generator PR: 1551
File: apps/generator/docs/generator-template.md:45-73
Timestamp: 2025-05-12T14:23:48.919Z
Learning: AsyncAPI 3.0.0 specification introduces significant structural changes from 2.x:
1. Operations become top-level elements with references to channels
2. Channels use 'address' for the topic path instead of having nested publish/subscribe
3. Messages are defined under a 'messages' container in channels
4. Servers can use a 'host' property
Applied to files:
.changeset/slack-auto-routing.mdpackages/helpers/test/__fixtures__/asyncapi-websocket-query.yml
🧬 Code graph analysis (2)
packages/templates/clients/websocket/python/components/Constructor.js (2)
packages/templates/clients/websocket/python/components/ClientClass.js (1)
receiveOperations(18-18)packages/components/src/components/OperationsDiscriminators.js (1)
OperationsDiscriminators(22-42)
packages/helpers/src/discriminators.js (2)
packages/components/src/components/OperationsDiscriminators.js (1)
operationDiscriminators(33-33)apps/keeper/src/index.js (1)
operation(46-46)
🔇 Additional comments (12)
.changeset/slack-auto-routing.md (1)
1-10: Changeset is well-structured and appropriately versioned.The minor version bumps for both packages are appropriate for these additive, non-breaking feature changes. The description clearly summarizes the new components and helpers.
packages/helpers/src/index.js (1)
3-4: LGTM!The new discriminator helpers are correctly imported and exported, maintaining consistency with the existing module structure.
Also applies to: 30-32
packages/components/src/components/OnMessage.js (1)
36-39: Avoid bareexcept:clause in Python.Using a bare
except:catches all exceptions includingKeyboardInterruptandSystemExit, which is generally considered bad practice. Additionally, the indentation appears inconsistent with the rest of the method (6 spaces vs 2 spaces).🐛 Proposed fix
- # Parse message for routing - try: - parsed_message = json.loads(message) - except: - parsed_message = message + # Parse message for routing + try: + parsed_message = json.loads(message) + except (ValueError, TypeError): + parsed_message = messageLikely an incorrect or invalid review comment.
packages/components/src/components/OperationsDiscriminators.js (2)
4-11: Good structure for language extensibility.The
languageSpecificInitializersmap provides a clean extension point for adding other languages in the future. The JSDoc for this constant is clear.
33-35: Verify JSON serialization compatibility with Python syntax.
JSON.stringifyproduces valid JSON, but the output is embedded in Python code. This works because JSON strings use double quotes, which is valid Python syntax. However, if discriminator values ever contain characters that need different escaping in Python vs JSON (e.g., certain Unicode or control characters), this could produce invalid Python.For the current use case with simple string discriminators, this should work correctly.
packages/components/src/index.js (1)
21-21: LGTM!The new export follows the established pattern and correctly exposes the
OperationsDiscriminatorscomponent from the package's public API.packages/templates/clients/websocket/python/components/Constructor.js (1)
34-37: LGTM!The
OperationsDiscriminatorscomponent is correctly integrated with appropriate language and operations props. The component handles empty/undefined operations internally, so this usage is safe.packages/helpers/test/__fixtures__/asyncapi-websocket-query.yml (2)
77-106: Good test coverage for discriminator edge cases.The three new messages provide excellent coverage for testing the discriminator extraction logic:
MarketUpdateMessage: Valid discriminator with const valueMessageWithoutDiscriminator: No discriminator (should be skipped)MessageWithDiscriminatorNoConst: Discriminator without const (edge case)This aligns well with the PR discussion about handling cases where users must provide both discriminator_key and discriminator_value for non-default cases.
197-205: LGTM!The new
receiveMarketUpdateoperation correctly references all three test messages and provides a realistic test case for the discriminator-based routing feature.packages/components/test/components/OperationsDiscriminators.test.js (1)
1-7: LGTM!Imports and setup are appropriate. The fixture path resolution correctly navigates to the shared test fixtures.
packages/helpers/test/discriminators.test.js (2)
20-57: Good test coverage for edge cases.The test cases thoroughly cover the expected behavior: valid discriminator extraction, and returning
nullfor messages without discriminators, missing const values, non-object payloads, and mismatched operations.
97-100: No action needed; the function has a clear contract expecting a valid array parameter.The function immediately calls
.forEach()on theoperationsparameter without guards, so null or undefined inputs will naturally throw an error. This is intentional design, not a missing feature. The existing test coverage—including the empty array case—is appropriate for the function's actual behavior and contract.Likely an incorrect or invalid review comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@derberg I did some refactoring as per your suggestions the only thing I didn't really get is are we also releasing minor version of generator? And in changeset I need to explain the approach with limitations? |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @packages/helpers/src/discriminators.js:
- Around line 46-64: getMessageDiscriminatorsFromOperations currently assumes
operations is iterable and will throw if operations is null/undefined; guard
against that by treating a falsy operations as an empty array (e.g., default
parameter or early return) before calling operations.forEach(), ensuring the
function returns an empty array when operations is null/undefined; update the
getMessageDiscriminatorsFromOperations signature/body to check operations (or
use operations = operations || []) and keep the rest of the logic unchanged so
existing references like operation.id(), operation.messages(), and
getMessageDiscriminatorData(message, operationId) remain intact.
In @packages/templates/clients/websocket/python/README.md:
- Around line 69-87: The numbered list is broken because the discriminator
guidance paragraph begins with "1." instead of continuing the steps; change that
line to a non-numbered note (e.g., convert to a paragraph or blockquote) and
preserve the content explaining discriminator_key, discriminator_value, and
register_handlers; ensure formatting matches other notes in the README (use the
same blockquote style used elsewhere) so it no longer appears as step "1." and
does not interrupt the numbered sequence.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
packages/components/test/components/__snapshots__/Connect.test.js.snapis excluded by!**/*.snappackages/components/test/components/__snapshots__/OnMessage.test.js.snapis excluded by!**/*.snappackages/templates/clients/websocket/test/integration-test/__snapshots__/integration.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
packages/components/src/components/OnMessage.jspackages/helpers/src/discriminators.jspackages/templates/clients/websocket/python/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/components/src/components/OnMessage.js
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/pr-review-checklist.md)
Write clear and consistent JSDoc comments for functions, including parameter types, return values, and error conditions
Files:
packages/helpers/src/discriminators.js
🧠 Learnings (1)
📚 Learning: 2026-01-05T09:57:34.800Z
Learnt from: CR
Repo: asyncapi/generator PR: 0
File: .github/pr-review-checklist.md:0-0
Timestamp: 2026-01-05T09:57:34.800Z
Learning: Applies to **/*.{js,ts,jsx,tsx} : Write clear and consistent JSDoc comments for functions, including parameter types, return values, and error conditions
Applied to files:
packages/helpers/src/discriminators.js
🔇 Additional comments (1)
packages/helpers/src/discriminators.js (1)
1-38: LGTM!The function correctly handles all null/undefined cases for payload, discriminator, properties, and const value. JSDoc documentation is present and clear per coding guidelines.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/templates/clients/websocket/python/README.md:
- Line 72: The README's phrase "follow steps 1-3 for the basic Slack client
above" is ambiguous because the basic Slack section isn't numbered; update the
Slack Client example with auto-routing to either (a) explicitly list the three
required setup steps (e.g., install deps, set env vars, run the client) inline,
or (b) change the reference to point to a clear heading or section name (e.g.,
"see 'Basic Slack client' section above") so readers know exactly which
instructions to follow; edit the sentence in
packages/templates/clients/websocket/python/README.md under the Slack Client
auto-routing paragraph to include the explicit step list or a clear section
reference.
🧹 Nitpick comments (1)
packages/templates/clients/websocket/python/README.md (1)
87-87: Consider improving clarity and placement of technical details.This technical note about discriminator behavior may be difficult for users to understand without concrete examples. Additionally, it's placed within a quickstart/example section rather than API documentation.
Consider these improvements:
📚 Suggested improvements
Option 1: Add a concrete example to clarify
-1. By default, `discriminator_key` is derived from the discriminator field, and `discriminator_value` from the corresponding `const` in the AsyncAPI document. For non-default cases, users must provide both `discriminator_key` and `discriminator_value` explicitly in register_handlers. Partial inputs are not supported to avoid ambiguity in message routing. +### Handler Registration Notes + +By default, the client derives `discriminator_key` and `discriminator_value` automatically from the AsyncAPI document's discriminator field and message `const` values. + +If you need to override these defaults (for example, when handling messages without a `const` value), you must provide both `discriminator_key` and `discriminator_value` explicitly when calling `register_handlers`. Providing only one is not supported, as it would create ambiguous routing rules (multiple messages could match the same discriminator key). + +Example: See `example-slack-with-routing.py` for the default (automatic) behavior.Option 2: Move to a separate API documentation section
Consider moving this technical detail to a dedicated "API Reference" or "Advanced Usage" section at the end of the README, keeping the quickstart focused on getting the example running.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/templates/clients/websocket/python/README.md
🔇 Additional comments (1)
packages/templates/clients/websocket/python/README.md (1)
70-87: No action needed. The file does not contain duplicate "Client for Slack with Auto-Routing" sections. A search of the file confirms only one occurrence of this section header exists.Likely an incorrect or invalid review comment.
|
|
||
| ## Client for Slack with Auto-Routing | ||
|
|
||
| To run the Slack Client example with auto-routing, follow steps 1-3 for the basic Slack client above, then: |
There was a problem hiding this comment.
Clarify which steps to follow.
The reference to "follow steps 1-3 for the basic Slack client above" is ambiguous. The basic Slack section (line 16) itself refers to "the steps above," and there's no clearly numbered "steps 1-3" specific to the Slack client.
Consider being more explicit:
📝 Suggested clarification
-To run the Slack Client example with auto-routing, follow steps 1-3 for the basic Slack client above, then:
+To run the Slack Client example with auto-routing, complete the setup for the basic Slack client (lines 24-52 in the "Client for Slack" section above), then:Or alternatively:
-To run the Slack Client example with auto-routing, follow steps 1-3 for the basic Slack client above, then:
+To run the Slack Client example with auto-routing, first follow the "Client for Slack" setup above (generate credentials and install dependencies), then:🤖 Prompt for AI Agents
In @packages/templates/clients/websocket/python/README.md at line 72, The
README's phrase "follow steps 1-3 for the basic Slack client above" is ambiguous
because the basic Slack section isn't numbered; update the Slack Client example
with auto-routing to either (a) explicitly list the three required setup steps
(e.g., install deps, set env vars, run the client) inline, or (b) change the
reference to point to a clear heading or section name (e.g., "see 'Basic Slack
client' section above") so readers know exactly which instructions to follow;
edit the sentence in packages/templates/clients/websocket/python/README.md under
the Slack Client auto-routing paragraph to include the explicit step list or a
clear section reference.
5 participants
Description
This is for now a draft and more cleanup need to be done so that other examples are not affected but it is working for slack perfectly.
Related issue(s)
Fixes #1548
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.