Conversation
WalkthroughThis change centralizes the logic for finding available network ports by moving the Changes
Sequence Diagram(s)sequenceDiagram
participant UI
participant RustExecutor
participant Utils
UI->>RustExecutor: Start initialization with config
RustExecutor->>Utils: find_and_set_port for gql_port (4000-40000)
RustExecutor->>Utils: find_and_set_port for hc_admin_port (2000-40000)
RustExecutor->>Utils: find_and_set_port for hc_app_port (1337-40000)
Utils-->>RustExecutor: Return available ports or error
RustExecutor-->>UI: Continue startup with assigned ports
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. 🪧 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
rust-executor/Cargo.toml(1 hunks)rust-executor/src/lib.rs(3 hunks)rust-executor/src/utils.rs(1 hunks)ui/src-tauri/Cargo.toml(0 hunks)ui/src-tauri/src/lib.rs(1 hunks)ui/src-tauri/src/util.rs(0 hunks)
💤 Files with no reviewable changes (2)
- ui/src-tauri/Cargo.toml
- ui/src-tauri/src/util.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
ui/src-tauri/src/lib.rs (1)
rust-executor/src/utils.rs (1)
find_port(9-20)
🔇 Additional comments (7)
rust-executor/Cargo.toml (1)
119-119: Successfully added portpicker dependency.The portpicker crate has been correctly added to support the port-scanning functionality implemented in the utils module.
ui/src-tauri/src/lib.rs (2)
14-14: Import correctly updated to use centralized implementation.The
find_portimport has been correctly updated to use the implementation fromrust_executor::utilsinstead of the local implementation, supporting the PR's goal of centralizing this functionality.
190-190: Function call adjusted to use imported implementation.Line 190 correctly uses the imported
find_portfunction with a reasonable port range (12000-13000), which should avoid conflicts with common system services.rust-executor/src/utils.rs (1)
3-3: Import correctly added for the portpicker crate.The portpicker import has been correctly added to support the new port scanning functionality.
rust-executor/src/lib.rs (3)
12-12: Module correctly made public.Making the utils module public allows external crates like the UI to import and use the
find_portfunction, supporting the centralization objective.
37-37: Import added for the find_port function.The function is correctly imported into the main crate scope to be used in the run function.
128-139: Default port assignment using find_port.These conditional blocks correctly implement the PR objective by ensuring all required ports are assigned values by scanning for available ports, addressing the port collision crash issue.
However, if the
find_portfunction is updated to return a Result as suggested, this code would need to be updated to handle potential errors rather than propagating panics.When updating this code to handle a Result return type from find_port, consider a pattern like:
if config.gql_port.is_none() { config.gql_port = Some(find_port(4000, 40000).unwrap_or_else(|err| { error!("Failed to find free port for GraphQL: {}", err); // Provide a fallback or exit gracefully std::process::exit(1); })); }
but keep an idempotent try to avoid problems with our (to be refactored) JS event loop
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
executor/src/main.ts (3)
8-8: Consider updating or removing the 'get-port' dependency.The PR description states that the get-port library "no longer functions correctly and throws errors when the preferred port is already in use." While you're still using it here for the event loop hack, it might be better to:
- Replace it with the new Rust-based port finding solution, or
- Add a comment explaining why this dependency is still needed despite its issues.
This would avoid potential confusion for future developers.
89-100: The workaround for the JS event loop should be temporary.The current implementation is a good short-term solution to keep the JS event loop active. However:
- The comment clearly explains the workaround nature of this code, which is excellent.
- The try/catch block properly prevents crashes if port 50000 is already in use.
Consider adding a TODO comment with a ticket number to track when this workaround can be removed during the mentioned upcoming refactoring of the remaining JS code to Rust.
// So this here is a hack that works for now. +// TODO: Remove this hack when the remaining JS code is moved to Rust (issue #XXX) // Putting it in a try/catch block to avoid the process from crashing if the port is already in use.
102-102: Remove commented-out delay code.The commented-out delay line should be removed as it's no longer needed and might confuse future developers.
- //await new Promise(resolve => setTimeout(resolve, 1000));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG(1 hunks)executor/src/main.ts(1 hunks)rust-executor/src/js_core/mod.rs(1 hunks)rust-executor/src/lib.rs(4 hunks)rust-executor/src/utils.rs(1 hunks)ui/src-tauri/src/lib.rs(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- rust-executor/src/js_core/mod.rs
- CHANGELOG
🚧 Files skipped from review as they are similar to previous changes (3)
- ui/src-tauri/src/lib.rs
- rust-executor/src/utils.rs
- rust-executor/src/lib.rs
🔇 Additional comments (1)
executor/src/main.ts (1)
86-87: LGTM: Correctly removed unused parameters.Good cleanup - removing the unused destructured variables (
mocks,gqlPort,runDappServer,dAppPort) from the function parameters since port management has been moved to the Rust side.
Co-authored-by: James <34165044+jhweir@users.noreply.github.com>
2 participants
Tests revealed crashes on some machines where our default ports (1337, 2000) are used by other services. The JS lib get-port in the remaining JS executor implementation doesn't work (anymore?) and instead throws when the preferred port is used. This does free port scanning in Rust.
Summary by CodeRabbit
New Features
Bug Fixes
Chores