Conversation
The `Port` name was overloaded: it referred to both node connection ports and the platform abstraction trait. Rename the platform abstraction to `Platform` to disambiguate: - `Port` trait → `Platform` trait - `PortError` → `PlatformError` - `TestPort` → `TestPlatform` - `DesktopPort` → `DesktopPlatform` - `port.rs` → `platform.rs` - `desktop_port.rs` → `desktop_platform.rs` https://claude.ai/code/session_01MVWg5jAgGpHUSLpY2cjrKW
The alias was needed to disambiguate nodebox_core::node::Port from nodebox_core::port::Port. Since the latter is now Platform, plain Port is unambiguous. https://claude.ai/code/session_01MVWg5jAgGpHUSLpY2cjrKW
There was a problem hiding this comment.
Pull request overview
Renames the core I/O abstraction from Port to Platform across nodebox-core and nodebox-desktop to avoid confusion with the existing NodeBox node Port type.
Changes:
- Renamed
nodebox_core::portmodule/Porttrait/PortErrortonodebox_core::platform/Platform/PlatformError(includingTestPlatform). - Updated desktop app, render worker, parameter panel, and tests to use the new
Platformabstraction. - Updated docs and plans to reflect the new terminology.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/server-api.md | Updates web implementation notes/examples from WebPort → WebPlatform. |
| docs/rust-translation-plan.md | Updates architecture docs to reference platform.rs and nodebox-core::platform. |
| crates/nodebox-desktop/tests/file_tests.rs | Switches tests to Platform/TestPlatform imports and trait objects. |
| crates/nodebox-desktop/tests/cancellation_tests.rs | Updates cancellation tests to use Platform/TestPlatform. |
| crates/nodebox-desktop/src/render_worker.rs | Render worker now carries Arc<dyn Platform> through requests and draining logic. |
| crates/nodebox-desktop/src/parameter_panel.rs | Updates UI file dialog + sandbox error handling to Platform* types. |
| crates/nodebox-desktop/src/lib.rs | Renames module + re-export from DesktopPort to DesktopPlatform; updates run() wiring. |
| crates/nodebox-desktop/src/eval.rs | Evaluation functions and tests now take Arc<dyn Platform>. |
| crates/nodebox-desktop/src/desktop_platform.rs | Renames and retypes the desktop implementation to DesktopPlatform: Platform. |
| crates/nodebox-desktop/src/app.rs | App now stores/accepts Arc<dyn Platform> and updates docs/messages accordingly. |
| crates/nodebox-desktop/Cargo.toml | Updates comment to “DesktopPlatform dependencies”. |
| crates/nodebox-core/src/platform.rs | Renames the trait, error type, and test implementation to Platform-based naming. |
| crates/nodebox-core/src/lib.rs | Exposes pub mod platform; and updates crate-level docs. |
Comments suppressed due to low confidence (1)
crates/nodebox-core/src/platform.rs:334
- Doc comment still says "Errors that can occur during Port operations" even though the enum is now
PlatformError. Update the wording to avoid confusion during the Port→Platform rename.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Create a test platform and project context for evaluation tests. | ||
| fn test_port_and_context() -> (Arc<dyn nodebox_core::platform::Platform>, ProjectContext) { | ||
| (Arc::new(TestPlatform::new()), ProjectContext::new_unsaved()) | ||
| } |
There was a problem hiding this comment.
Test helper is still named test_port_and_context even though it now constructs a TestPlatform and returns a Platform. Renaming it (and usages) would keep terminology consistent after the Port→Platform rename.
| /// Create a test platform and project context for evaluation tests. | ||
| fn test_port_and_context() -> (Arc<dyn Platform>, ProjectContext) { | ||
| (Arc::new(TestPlatform::new()), ProjectContext::new_unsaved()) |
There was a problem hiding this comment.
Helper is still named test_port_and_context even though it now returns a Platform and the doc comment says "test platform". Renaming the function (and call sites) would make the intent consistent with the Port→Platform rename.
| /// Create a test platform and project context for evaluation tests. | ||
| fn test_port_and_context() -> (Arc<dyn Platform>, ProjectContext) { | ||
| (Arc::new(TestPlatform::new()), ProjectContext::new_unsaved()) | ||
| } |
There was a problem hiding this comment.
Helper is still named test_port_and_context even though it now returns a Platform and the doc comment says "test platform". Renaming the function (and call sites) would make the intent consistent with the Port→Platform rename.
Rename "Port" platform abstraction to "Platform". There is already a NodeBox Port struct, so this avoids confusion.