fix(engine): remove unused routing ports from actions and update asset download logic #689
Conversation
WalkthroughThe pull request introduces significant modifications to the routing logic of the Changes
Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
✅ Deploy Preview for reearth-flow canceled.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
engine/worker/src/types/metadata.rs (1)
26-30: LGTM! Consider adding unit tests.The implementation follows Rust conventions and correctly delegates to the underlying Vec's is_empty method. However, unit tests would help ensure the behavior remains correct.
Would you like me to help generate unit tests for this new method?
engine/worker/src/asset.rs (2)
14-16: LGTM! Consider adding debug logging.The early return optimization for empty assets is a good improvement. Consider adding debug logging to help with observability.
if asset.is_empty() { + tracing::debug!("Skipping download for empty asset"); return Ok(()); }
Line range hint
17-54: Consider adding performance safeguards.The concurrent download implementation could benefit from some protective measures:
- Consider limiting the number of concurrent downloads to prevent resource exhaustion
- Add timeout handling for individual downloads
Example approach using
futures::stream::StreamExt:use futures::stream::{self, StreamExt}; use tokio::time::timeout; use std::time::Duration; // ... in download_asset function let stream = stream::iter(uris) .map(|(name, uri)| async move { // Add timeout for individual downloads timeout(Duration::from_secs(30), async move { // ... existing download logic ... }).await?? }) .buffer_unordered(10); // Limit concurrent downloads stream.collect::<Vec<_>>().await
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
engine/docs/mdbook/src/action.mdis excluded by!**/*.md
📒 Files selected for processing (9)
engine/runtime/runtime/src/node.rs(2 hunks)engine/schema/actions.json(2 hunks)engine/schema/actions_en.json(2 hunks)engine/schema/actions_es.json(2 hunks)engine/schema/actions_fr.json(2 hunks)engine/schema/actions_ja.json(2 hunks)engine/schema/actions_zh.json(2 hunks)engine/worker/src/asset.rs(1 hunks)engine/worker/src/types/metadata.rs(1 hunks)
🔇 Additional comments (8)
engine/runtime/runtime/src/node.rs (1)
414-414: Verify the necessity of routing_port parameter
The get_input_ports() now returns an empty vector, but InputRouter struct still requires routing_port parameter. This parameter appears to be unused since no input ports are defined.
engine/schema/actions_zh.json (1)
2089-2089: Schema changes align with implementation
The removal of ports from both InputRouter and OutputRouter in the schema matches the Rust implementation changes.
Also applies to: 2348-2348
engine/schema/actions_ja.json (1)
2089-2089: Schema changes maintain localization consistency
The removal of ports from both InputRouter and OutputRouter in the Japanese schema maintains consistency with other localizations.
Also applies to: 2348-2348
engine/schema/actions.json (2)
2348-2348:
Verify the OutputRouter's port configuration
The OutputRouter's parameter schema requires a routingPort but its outputPorts array is empty. This creates an inconsistency in the routing logic.
2089-2089:
Verify the InputRouter's port configuration
The InputRouter's parameter schema requires a routingPort but its inputPorts array is empty. This creates an inconsistency in the routing logic.
engine/schema/actions_en.json (1)
2089-2089: Changes match the main schema file
The modifications to InputRouter and OutputRouter processors match those in actions.json.
Also applies to: 2348-2348
engine/schema/actions_es.json (1)
2089-2089: Changes match the main schema file
The modifications to InputRouter and OutputRouter processors match those in actions.json.
Also applies to: 2348-2348
engine/schema/actions_fr.json (1)
2348-2348:
Verify the removal of output ports from OutputRouter
The removal of output ports from the OutputRouter processor is a breaking change that affects how data flows out of sub-workflows. This change needs to be coordinated with any workflows using the OutputRouter.
Run the following script to find potentially affected workflows:
Overview
What I've done
What I haven't done
How I tested
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit
InputRouterandOutputRouter.