chore(websocket): update websocket server to yrs-warp #786
Conversation
WalkthroughThis pull request integrates extensive configuration and functionality updates across the project. It adds new ignore rules, workspace configurations, and metadata in Cargo.toml files; introduces new GraphQL types, queries, mutations, and resolvers for document management; and implements a gRPC authentication service with associated proto definitions. The websocket package is restructured with new modules, FFI interfaces, Dockerfile improvements, and updated CI workflows. Additionally, many legacy files are removed to streamline the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthService
participant JWTValidator
Client->>AuthService: VerifyAPIToken(token)
AuthService->>JWTValidator: Create new validator (config)
JWTValidator-->>AuthService: Validator instance (or error)
AuthService->>JWTValidator: Validate stripped token
JWTValidator-->>AuthService: Success/Failure
AuthService-->>Client: APITokenVerifyResponse (authorized: true/false)
sequenceDiagram
participant C
participant FFI
participant ConfigParser
participant DataStore
C->>FFI: get_latest_document(doc_id, config_json)
FFI->>ConfigParser: Parse configuration JSON
ConfigParser-->>FFI: Return Config struct
FFI->>DataStore: Retrieve latest document (from GCS/Redis)
DataStore-->>FFI: Document data (content, clock, timestamp)
FFI->>C: Return JSON string with document details
Possibly related PRs
Suggested reviewers
Poem
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: 15
🧹 Nitpick comments (22)
websocket/.gitignore (1)
1-2: Consider retaining IDE and system-specific ignores.While the current entries correctly ignore Rust build artifacts and output directories, consider maintaining common IDE and system-specific ignores to prevent accidental commits. This helps maintain a clean repository across different development environments.
/target/ /output/ +.idea/ +.vscode/ +**/*.rs.bk +.DS_Storewebsocket/Dockerfile (1)
6-6: Optimize build process with dependency caching and explicit configuration.The current build process can be improved for faster builds and better configurability.
+# Cache dependencies +COPY Cargo.toml Cargo.lock ./ +RUN mkdir src && \ + echo "fn main() {}" > src/main.rs && \ + cargo build --release && \ + rm -rf src + +# Build actual application +COPY src src/ RUN cargo build --releaseConsider adding build arguments for conditional compilation:
ARG RUST_ENV=production ENV RUST_ENV=${RUST_ENV}websocket/docker-compose.yml (1)
30-31: Consider documenting volume persistence requirements.The volume configuration is suitable for development, but consider adding comments about data persistence requirements and any specific volume configurations needed for production deployments.
Add a comment above the volumes section:
+# Define persistent volumes for service data volumes: gcs-data:websocket/benches/readme.adoc (3)
1-8: Add version and metadata information.Consider adding the following metadata to improve documentation maintainability:
- Document version/last updated date
- Author information
- Related documentation links
= Benchmark Suite :toc: :toc-placement!: +:revnumber: 1.0.0 +:revdate: 2024-01 +:author: Your Team +:email: [email protected] +:related-docs: README.md, CONTRIBUTING.md
13-25: Enhance running instructions with prerequisites and examples.The running instructions should be more comprehensive to help users get started quickly and troubleshoot issues.
Add the following sections:
== Running Benchmarks + +=== Prerequisites +* Rust toolchain (minimum version: X.Y.Z) +* Cargo criterion installed +* Sufficient system resources (recommended: 4GB RAM, 2 CPU cores) To run all benchmarks: [source,bash] ---- cargo bench ---- + +=== Running Specific Benchmarks +[source,bash] +---- +# Run only connection tests +cargo bench connection + +# Run with custom parameters +cargo bench -- --sample-size 100 +---- + +=== Troubleshooting +* If benchmarks fail to connect, ensure the WebSocket server is running +* For "port in use" errors, check for other running instances Results will be saved in `target/criterion/` directory, including:
83-111: Enhance configuration and results interpretation sections.The configuration and interpretation sections should provide more detailed guidance for consistent and meaningful benchmarking.
Add the following sections:
- Environment-specific configurations
- CI/CD integration
- Cross-environment comparison guidelines
Example additions:
== Configuration The benchmarks are configured with: + +=== Environment Variables +[source,bash] +---- +BENCHMARK_SERVER_URL=ws://localhost:8080 # WebSocket server URL +BENCHMARK_TIMEOUT=30 # Timeout in seconds +BENCHMARK_LOG_LEVEL=info # Logging verbosity +---- + +=== CI/CD Integration +[source,yaml] +---- +# Example GitHub Actions workflow +benchmark: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - name: Run benchmarks + run: cargo bench + - name: Store results + uses: actions/upload-artifact@v2 + with: + name: benchmark-results + path: target/criterion/ +---- + +=== Cross-Environment Comparisons +When comparing results across different environments: +* Document hardware specifications +* Note operating system and version +* Record background processes +* Run benchmarks multiple times +* Use statistical significance testswebsocket/src/broadcast_pool.rs (1)
100-100: Consider handling the case when the group is not found inremove_connection.If
self.groups.get(doc_id)returnsNone, the method silently does nothing. It might be beneficial to log this case or handle it explicitly to avoid potential inconsistencies.Apply this diff to add a log message:
if let Some(group) = self.groups.get(doc_id) { let remaining = group.decrement_connections(); if remaining == 0 { self.cleanup_empty_groups().await; } +} else { + tracing::warn!("Attempted to remove connection for non-existent doc_id: {}", doc_id); }websocket/src/bin/main.rs (3)
53-56: Handle potential configuration loading failures more robustly.The current implementation logs an error message and proceeds with the default configuration if loading the configuration fails. Consider exiting the application or implementing a fallback strategy depending on the criticality of the configuration.
Alternatively, ensure that the user is adequately informed about the configuration issues and any implications of using defaults.
74-77: Consider configuring the buffer capacity for theBroadcastPool.The
BroadcastPoolis initialized with the default buffer capacity of 1024. Depending on the expected load, it might be beneficial to make this configurable via theConfigstruct.Adjust the initialization as follows:
let pool = Arc::new(BroadcastPool::new(store, config.redis)); +// Alternatively, if buffer_capacity is added to Config: +// let pool = Arc::new(BroadcastPool::with_buffer_capacity(store, config.redis, config.buffer_capacity)); tracing::info!("Broadcast pool initialized");And update the
Configstruct to includebuffer_capacityif necessary.
90-96: Simplifydoc_idnormalization logic inws_handler.The current logic checks if the
doc_idends with":main"and trims it. Ensure that this behavior is intended and consider documenting the reason. If this is a convention, perhaps using a more robust pattern matching or a configuration parameter would be clearer.Alternatively, explain why this normalization is necessary or if other suffixes should be handled similarly.
websocket/src/broadcast.rs (3)
223-227: Handle Redis connection initialization errors more gracefully.Currently, any Redis connection error will be propagated upwards. Consider providing more context in the error message or implementing a retry mechanism.
Modify the error handling to include context:
async fn init_redis_connection( url: &str, ) -> Result<Arc<Mutex<RedisConnection>>, redis::RedisError> { let client = redis::Client::open(url)?; let conn = client.get_multiplexed_async_connection().await.map_err(|e| { + tracing::error!("Failed to establish Redis connection to {}: {}", url, e); e })?; Ok(Arc::new(Mutex::new(conn))) }
251-262: Improve error logging when loading from storage fails.When
load_from_storagefails, the error is logged but not acted upon. Consider whether fallback mechanisms or user notifications are necessary.If no further action is needed, ensure that the logging provides sufficient detail for debugging.
match store.load_doc(doc_name, &mut txn).await { Ok(_) => { tracing::debug!("Successfully loaded document '{}' from storage", doc_name); } Err(e) => { tracing::error!("Failed to load document '{}' from storage: {}", doc_name, e); + // Potentially initiate a new document or notify the user } }
423-433: Ensure proper cleanup inDropimplementation ofBroadcastGroup.While aborting the
awareness_updatertask inDrop, make sure that all associated resources are properly released to prevent memory leaks.Confirm that any other spawned tasks or opened connections are also properly terminated.
websocket/benches/server_bench.rs (4)
8-13: IncreaseTIMEOUT_DURATIONto prevent unintended timeoutsThe
TIMEOUT_DURATIONis set to 50 milliseconds, which may be too short for network operations. This can lead to unintended timeouts, especially under variable network conditions or system load. Consider increasing the timeout duration to ensure reliable benchmark results.Apply this diff to adjust the timeout duration:
-const TIMEOUT_DURATION: Duration = Duration::from_millis(50); +const TIMEOUT_DURATION: Duration = Duration::from_millis(200);
79-93: Reuse the Tokio runtime across benchmark functionsEach benchmark function creates a new Tokio runtime, which introduces unnecessary overhead and can affect the accuracy of the benchmarks. Reusing a single runtime across all benchmark functions improves efficiency and resource utilization.
Consider initializing a global runtime and reusing it:
+lazy_static::lazy_static! { + static ref RUNTIME: tokio::runtime::Runtime = Builder::new_current_thread().enable_all().build().unwrap(); +} fn bench_websocket_connection(c: &mut Criterion) { - let rt = Builder::new_current_thread().enable_all().build().unwrap(); c.bench_function("websocket_connection", |b| { b.iter(|| { - rt.block_on(async { + RUNTIME.block_on(async { let ws_stream = connect_client("bench-doc").await; // Properly close the connection close_connection(ws_stream).await; // Small delay between connections sleep(Duration::from_millis(10)).await; }); }) }); }Apply similar changes to other benchmark functions to use the shared runtime.
44-49: Handle connection errors without panickingIn the
connect_clientfunction, using.expect("Failed to connect")will cause a panic if the connection fails. This could stop the entire benchmarking process if the server is temporarily unreachable. Handling the error gracefully allows the benchmarks to continue running or retry the connection.Modify the function to return a
Result:-async fn connect_client(doc_id: &str) -> tokio_tungstenite::WebSocketStream<tokio_tungstenite::MaybeTlsStream<tokio::net::TcpStream>> { +async fn connect_client(doc_id: &str) -> Result<tokio_tungstenite::WebSocketStream<tokio_tungstenite::MaybeTlsStream<tokio::net::TcpStream>>, Box<dyn std::error::Error>> { let url = format!("ws://127.0.0.1:8080/{}", doc_id); - let (ws_stream, _) = connect_async(url).await.expect("Failed to connect"); - ws_stream + let (ws_stream, _) = connect_async(url).await?; + Ok(ws_stream) }Update the calling code to handle the
Resultaccordingly.
480-486: Reevaluate Criterion configuration for reliable measurementsThe current Criterion configuration sets
sample_sizeto 10 andmeasurement_timeto 1 second. These values might be insufficient for capturing reliable benchmark data, especially for operations with minimal execution times.Consider adjusting the configuration to improve measurement accuracy:
fn configure_criterion() -> Criterion { Criterion::default() - .sample_size(10) - .measurement_time(Duration::from_secs(1)) + .sample_size(50) + .measurement_time(Duration::from_secs(5)) .warm_up_time(Duration::from_millis(500)) .output_directory(std::path::Path::new("target/criterion")) }websocket/src/conf.rs (2)
14-22: Handle missing configuration file gracefullyIf
conf.yamldoes not exist, loading the configuration will result in an error, preventing the application from starting. To improve robustness, consider making the configuration file optional and relying on default settings when the file is missing.Modify the configuration loader to treat the file as optional:
let builder = ConfigLoader::builder() - .add_source(File::new("conf.yaml", FileFormat::Yaml)) + .add_source(File::new("conf.yaml", FileFormat::Yaml).required(false)) // Add default values as fallback .set_default("redis.url", "redis://127.0.0.1:6379")? .set_default("redis.ttl", 3600)? .set_default("gcs.bucket_name", "yrs-dev")? .set_default("gcs.endpoint", "http://localhost:4443")?;
27-40: SimplifyDefaultimplementation by deriving if possibleIf
RedisConfigandGcsConfigimplement theDefaulttrait, you can deriveDefaultforConfigto simplify the code and reduce manual implementation.Apply this change if the inner structs implement
Default:#[derive(Debug, Deserialize)] +#[derive(Debug, Deserialize, Default)] pub struct Config { pub redis: RedisConfig, pub gcs: GcsConfig, } -impl Default for Config { - fn default() -> Self { - Config { - redis: RedisConfig { - url: "redis://127.0.0.1:6379".to_string(), - ttl: 3600, - }, - gcs: GcsConfig { - bucket_name: "yrs-dev".to_string(), - endpoint: Some("http://localhost:4443".to_string()), - }, - } - } -}websocket/output/conf.yaml (1)
6-6: Add a newline at the end of the fileThe file does not end with a newline character. Adding a newline at the end of the file is a common convention and can prevent issues with certain tools or systems that expect this format.
Apply this diff to add a newline at the end of the file:
endpoint: http://localhost:4443 +🧰 Tools
🪛 yamllint (1.35.1)
[error] 6-6: no new line character at the end of file
(new-line-at-end-of-file)
websocket/Cargo.toml (1)
31-31: Remove commented dependencyThe commented out dependency
lmdb-rsshould either be removed or uncommented if needed.🧰 Tools
🪛 GitHub Actions: ci
[error] The file is not properly formatted according to taplo formatting standards
websocket/script/run.sh (1)
1-1: Consider documenting the yrs-warp migrationSince this is a significant refactoring to use yrs-warp:
- Consider adding migration notes or upgrade guide in README.md
- Document any breaking changes or API differences
- Add configuration examples for GCS and Redis integration
Would you like me to help generate the documentation for these changes?
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
websocket/.DS_Storeis excluded by!**/.DS_Storewebsocket/Cargo.lockis excluded by!**/*.lock,!**/*.lockwebsocket/README.mdis excluded by!**/*.md
📒 Files selected for processing (82)
websocket/.dockerignore(0 hunks)websocket/.gitignore(1 hunks)websocket/.taplo.toml(0 hunks)websocket/Cargo.toml(1 hunks)websocket/Dockerfile(1 hunks)websocket/LICENSE(1 hunks)websocket/Makefile.toml(0 hunks)websocket/README.adoc(1 hunks)websocket/Redis.Dockerfile(0 hunks)websocket/Websocket.Dockerfile(0 hunks)websocket/app/Cargo.toml(0 hunks)websocket/app/examples/readme.adoc(0 hunks)websocket/app/examples/room_client.rs(0 hunks)websocket/app/examples/session_client.rs(0 hunks)websocket/app/readme.adoc(0 hunks)websocket/app/src/bin/main.rs(0 hunks)websocket/app/src/config.rs(0 hunks)websocket/app/src/errors.rs(0 hunks)websocket/app/src/handlers/cleanup.rs(0 hunks)websocket/app/src/handlers/heartbeat.rs(0 hunks)websocket/app/src/handlers/message_handler.rs(0 hunks)websocket/app/src/handlers/mod.rs(0 hunks)websocket/app/src/handlers/room_handler.rs(0 hunks)websocket/app/src/handlers/socket_handler.rs(0 hunks)websocket/app/src/handlers/types.rs(0 hunks)websocket/app/src/lib.rs(0 hunks)websocket/app/src/middleware/mod.rs(0 hunks)websocket/app/src/middleware/trace.rs(0 hunks)websocket/app/src/room.rs(0 hunks)websocket/app/src/routes.rs(0 hunks)websocket/app/src/state.rs(0 hunks)websocket/benches/readme.adoc(1 hunks)websocket/benches/server_bench.rs(1 hunks)websocket/build.sh(1 hunks)websocket/clippy.toml(0 hunks)websocket/conf/conf.yaml(0 hunks)websocket/config/config.toml(0 hunks)websocket/crates/infra/Cargo.toml(0 hunks)websocket/crates/infra/src/lib.rs(0 hunks)websocket/crates/infra/src/persistence/editing_session.rs(0 hunks)websocket/crates/infra/src/persistence/event_handler.rs(0 hunks)websocket/crates/infra/src/persistence/gcs/gcs_client.rs(0 hunks)websocket/crates/infra/src/persistence/gcs/mod.rs(0 hunks)websocket/crates/infra/src/persistence/local_storage/local_client.rs(0 hunks)websocket/crates/infra/src/persistence/local_storage/mod.rs(0 hunks)websocket/crates/infra/src/persistence/mod.rs(0 hunks)websocket/crates/infra/src/persistence/project_repository.rs(0 hunks)websocket/crates/infra/src/persistence/redis/connection.rs(0 hunks)websocket/crates/infra/src/persistence/redis/default_key_manager.rs(0 hunks)websocket/crates/infra/src/persistence/redis/errors.rs(0 hunks)websocket/crates/infra/src/persistence/redis/flow_project_lock.rs(0 hunks)websocket/crates/infra/src/persistence/redis/flow_project_redis_data_manager.rs(0 hunks)websocket/crates/infra/src/persistence/redis/keys.rs(0 hunks)websocket/crates/infra/src/persistence/redis/mod.rs(0 hunks)websocket/crates/infra/src/persistence/redis/types.rs(0 hunks)websocket/crates/infra/src/persistence/redis/updates.rs(0 hunks)websocket/crates/infra/src/persistence/repository.rs(0 hunks)websocket/crates/infra/src/types/mod.rs(0 hunks)websocket/crates/infra/src/types/project.rs(0 hunks)websocket/crates/infra/src/types/snapshot.rs(0 hunks)websocket/crates/infra/src/types/user.rs(0 hunks)websocket/crates/infra/src/types/workspace.rs(0 hunks)websocket/crates/infra/src/utils.rs(0 hunks)websocket/crates/services/Cargo.toml(0 hunks)websocket/crates/services/examples/edit_session_service.rs(0 hunks)websocket/crates/services/examples/project_service.rs(0 hunks)websocket/crates/services/examples/readme.adoc(0 hunks)websocket/crates/services/src/error.rs(0 hunks)websocket/crates/services/src/lib.rs(0 hunks)websocket/crates/services/src/manage_project_edit_session.rs(0 hunks)websocket/crates/services/src/project.rs(0 hunks)websocket/crates/services/src/types.rs(0 hunks)websocket/docker-compose.yml(1 hunks)websocket/output/conf.yaml(1 hunks)websocket/output/run.sh(1 hunks)websocket/redis.conf(0 hunks)websocket/rustfmt.toml(0 hunks)websocket/script/run.sh(1 hunks)websocket/src/bin/main.rs(1 hunks)websocket/src/broadcast.rs(1 hunks)websocket/src/broadcast_pool.rs(1 hunks)websocket/src/conf.rs(1 hunks)
⛔ Files not processed due to max files limit (10)
- websocket/src/conf.yaml
- websocket/src/conn.rs
- websocket/src/lib.rs
- websocket/src/storage/gcs/mod.rs
- websocket/src/storage/kv/error.rs
- websocket/src/storage/kv/keys.rs
- websocket/src/storage/kv/mod.rs
- websocket/src/storage/mod.rs
- websocket/src/storage/sqlite/mod.rs
- websocket/src/ws.rs
💤 Files with no reviewable changes (66)
- websocket/clippy.toml
- websocket/crates/infra/src/utils.rs
- websocket/rustfmt.toml
- websocket/.taplo.toml
- websocket/.dockerignore
- websocket/redis.conf
- websocket/app/Cargo.toml
- websocket/config/config.toml
- websocket/crates/infra/src/persistence/event_handler.rs
- websocket/crates/infra/Cargo.toml
- websocket/crates/services/examples/readme.adoc
- websocket/Redis.Dockerfile
- websocket/app/readme.adoc
- websocket/crates/infra/src/persistence/redis/connection.rs
- websocket/app/src/bin/main.rs
- websocket/app/examples/room_client.rs
- websocket/crates/services/examples/edit_session_service.rs
- websocket/app/src/middleware/trace.rs
- websocket/app/src/errors.rs
- websocket/crates/infra/src/lib.rs
- websocket/crates/services/src/project.rs
- websocket/crates/infra/src/types/project.rs
- websocket/app/src/middleware/mod.rs
- websocket/crates/services/src/error.rs
- websocket/crates/infra/src/persistence/gcs/mod.rs
- websocket/app/src/handlers/mod.rs
- websocket/app/src/routes.rs
- websocket/crates/services/examples/project_service.rs
- websocket/crates/infra/src/persistence/editing_session.rs
- websocket/crates/infra/src/types/mod.rs
- websocket/Makefile.toml
- websocket/crates/infra/src/persistence/repository.rs
- websocket/crates/services/src/types.rs
- websocket/app/src/handlers/room_handler.rs
- websocket/crates/infra/src/persistence/mod.rs
- websocket/crates/infra/src/persistence/redis/default_key_manager.rs
- websocket/app/examples/readme.adoc
- websocket/app/src/handlers/heartbeat.rs
- websocket/Websocket.Dockerfile
- websocket/app/src/config.rs
- websocket/crates/infra/src/persistence/redis/mod.rs
- websocket/crates/services/src/lib.rs
- websocket/crates/infra/src/persistence/redis/types.rs
- websocket/app/src/handlers/types.rs
- websocket/crates/infra/src/persistence/redis/keys.rs
- websocket/crates/infra/src/persistence/local_storage/mod.rs
- websocket/app/src/handlers/message_handler.rs
- websocket/conf/conf.yaml
- websocket/app/src/handlers/cleanup.rs
- websocket/crates/infra/src/persistence/redis/flow_project_redis_data_manager.rs
- websocket/crates/infra/src/persistence/project_repository.rs
- websocket/app/src/handlers/socket_handler.rs
- websocket/crates/infra/src/types/user.rs
- websocket/crates/infra/src/types/snapshot.rs
- websocket/app/examples/session_client.rs
- websocket/app/src/lib.rs
- websocket/crates/infra/src/persistence/redis/errors.rs
- websocket/crates/infra/src/persistence/redis/flow_project_lock.rs
- websocket/crates/infra/src/persistence/redis/updates.rs
- websocket/app/src/room.rs
- websocket/crates/services/Cargo.toml
- websocket/crates/infra/src/persistence/gcs/gcs_client.rs
- websocket/app/src/state.rs
- websocket/crates/services/src/manage_project_edit_session.rs
- websocket/crates/infra/src/persistence/local_storage/local_client.rs
- websocket/crates/infra/src/types/workspace.rs
✅ Files skipped from review due to trivial changes (2)
- websocket/LICENSE
- websocket/README.adoc
🧰 Additional context used
🪛 yamllint (1.35.1)
websocket/output/conf.yaml
[error] 6-6: no new line character at the end of file
(new-line-at-end-of-file)
🪛 Shellcheck (0.10.0)
websocket/build.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
websocket/output/run.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
websocket/script/run.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🪛 GitHub Actions: ci
websocket/Cargo.toml
[error] The file is not properly formatted according to taplo formatting standards
🔇 Additional comments (8)
websocket/.gitignore (1)
2-2: Verify the purpose of the /output/ directory.The addition of
/output/suggests a new directory for generated files. Let's verify its usage in the codebase.✅ Verification successful
The
/output/directory is correctly ignoredThis is a build artifacts directory that contains the compiled binary, configuration files, and scripts. The .gitignore entry is appropriate as these files are recreatable through the build process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to the output directory rg -g '!.gitignore' 'output/' websocket/Length of output: 136
Script:
#!/bin/bash # Check the complete build script cat websocket/build.shLength of output: 189
websocket/Dockerfile (1)
11-11: Verify the correct binary name from Cargo.toml.Let's ensure we're using the correct binary name from the project configuration.
✅ Verification successful
Binary name in Dockerfile is correct
The binary name "websocket" in the Dockerfile matches the package name in Cargo.toml. When no explicit binary target is specified in Cargo.toml, Rust creates a binary with the same name as the package.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the binary name defined in Cargo.toml rg -A 5 '^\[\[bin\]\]' Cargo.toml || \ rg -A 5 '^name\s*=' Cargo.tomlLength of output: 170
Script:
#!/bin/bash # Check the binary name in websocket/Cargo.toml cat websocket/Cargo.toml | grep -A 5 '^\[\[bin\]\]' || \ cat websocket/Cargo.toml | grep -A 5 '^name\s*='Length of output: 344
websocket/docker-compose.yml (4)
1-1: LGTM! Version upgrade provides access to newer Docker Compose features.The upgrade to version "3.8" is a good choice as it provides access to newer Docker Compose features and improvements.
11-12: Security concern: GCS endpoint uses HTTP.The GCS endpoint is configured to use HTTP, which is insecure for production environments. While this is acceptable for the fake-gcs development server, ensure that production configurations use HTTPS.
Consider adding a comment in the docker-compose file to clarify that this is a development-only configuration.
22-28: LGTM! Fake GCS server configuration is well-structured.The fake GCS server configuration is appropriate for development:
- Uses a well-known emulator image
- Properly configured volume mounting for data persistence
- Port exposure is correctly configured
17-20: Verify Redis version compatibility.The Redis configuration looks good with Alpine-based image for smaller size. However, let's verify if Redis 7 is the latest stable version suitable for the project.
Run this script to check Redis version compatibility:
✅ Verification successful
Redis version is properly configured ✅
The current Redis 7.x configuration is optimal as it uses the latest major version while allowing automatic minor version updates (currently 7.4) within the stable release. The Alpine-based image is correctly used for minimal container size.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check latest Redis versions and any breaking changes curl -s https://registry.hub.docker.com/v2/repositories/library/redis/tags?page_size=10 | jq -r '.results[].name' | grep -v 'alpine' | grep -E '^[0-9]+$|^[0-9]+\.[0-9]+$'Length of output: 176
websocket/benches/server_bench.rs (1)
398-445: Ensure sufficient sample size and client counts for benchmark accuracyIn
bench_concurrent_clients_variable, reducing the sample size to 10 and omitting the test with 16 clients might compromise the statistical reliability of the benchmarks. Adequate sample sizes and a range of client counts are essential for meaningful performance evaluation.Please verify if the reduced sample size and client counts still provide accurate benchmark results. Consider restoring a higher sample size and including additional client counts for a comprehensive assessment.
websocket/Cargo.toml (1)
15-51: Review dependency version constraintsConsider using more specific version constraints for critical dependencies to ensure reproducible builds:
Also, verify the compatibility between yrs 0.21.3 and axum 0.7:
🧰 Tools
🪛 GitHub Actions: ci
[error] The file is not properly formatted according to taplo formatting standards
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
websocket/src/conf.yaml (1)
1-6: Add environment-specific configuration support.The configuration file would benefit from environment-specific overrides and validation rules.
Consider adding:
- Environment-specific configuration files (e.g.,
conf.dev.yaml,conf.prod.yaml)- Schema validation rules
- Required vs optional settings documentation
- Configuration version for tracking changes
+# Configuration version +version: 1.0.0 + +# Environment (development, staging, production) +environment: ${ENV:-development} + redis: # ... redis config ... gcs: # ... gcs config ... + +# Schema validation rules +schema: + required: + - redis.url + - gcs.bucket_name + optional: + - redis.tls_enabled + - gcs.credentials
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
websocket/Dockerfile(1 hunks)websocket/src/conf.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- websocket/Dockerfile
🔇 Additional comments (2)
websocket/src/conf.yaml (2)
1-3: Review Redis configuration security and documentation.The Redis configuration lacks authentication and SSL/TLS settings, which could pose security risks in production. Additionally, consider documenting the purpose of the TTL setting.
Consider the following improvements:
- Add authentication settings
- Enable SSL/TLS for secure communication
- Add configuration comments
- Make the URL configurable via environment variables
redis: - url: redis://redis:6379 + # Redis connection settings + # URL format: redis[s]://[[username][:password]@][host][:port][/database] + url: ${REDIS_URL:-redis://redis:6379} + # Time-to-live in seconds for cached items ttl: 3600 + # SSL/TLS configuration + tls_enabled: false + auth: + username: ${REDIS_USERNAME:-} + password: ${REDIS_PASSWORD:-}✅ Verification successful
Simplify Redis security configuration and improve documentation
The Redis configuration can be improved while maintaining compatibility with the existing docker-compose setup:
redis: + # Redis connection URL (defaults to docker-compose service URL) url: redis://redis:6379 + # Cache expiration time in seconds ttl: 3600 + # Security settings (recommended for production) + auth: + enabled: false # Set to true in production + username: "" # Configure in production + password: "" # Configure in production
- The
urlis already configurable viaREDIS_URLin docker-compose.yml- Add security settings but keep them disabled by default for development
- Include documentation for configuration options
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for Redis-related configuration files echo "=== Redis Configuration Files ===" fd -e yaml -e yml . | xargs rg -l 'redis:' echo -e "\n=== Redis Client Initialization ===" # Look for Redis client initialization code rg -i "redis" --type ts --type js -C 2 echo -e "\n=== Environment Variable Usage ===" # Check for environment variable usage rg -i "process\.env\.REDIS" --type ts --type js -C 2Length of output: 438
Script:
#!/bin/bash # Check the docker-compose configuration echo "=== Docker Compose Redis Configuration ===" cat websocket/docker-compose.yml echo -e "\n=== Broader Redis Code Search ===" # Search for Redis-related code with broader patterns rg -i "redis|cache" --type ts --type js --type typescript -C 2 echo -e "\n=== Redis-Related Files ===" # Look for Redis-related files fd -e ts -e js . | rg -i "redis|cache"Length of output: 930
4-6: Secure GCS configuration for production use.The current GCS configuration uses HTTP and lacks authentication, which is unsuitable for production. Additionally, the bucket name suggests this is for development use only.
Consider the following improvements:
- Use HTTPS endpoint
- Add GCS authentication configuration
- Make settings configurable via environment variables
- Add configuration comments
gcs: - bucket_name: yrs-dev - endpoint: http://fake-gcs:4443 + # Google Cloud Storage configuration + bucket_name: ${GCS_BUCKET_NAME:-yrs-dev} + # GCS endpoint (use storage.googleapis.com for production) + endpoint: ${GCS_ENDPOINT:-https://storage.googleapis.com} + # Authentication + credentials: + type: service_account + path: ${GCS_CREDENTIALS_PATH:-}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (9)
websocket/docker-compose.yml (3)
13-15: Add health checks and restart policy for production readiness.While the dependencies are correctly specified, consider adding:
- Health checks to ensure services are truly ready
- Restart policy for automatic recovery
depends_on: - redis - fake-gcs + healthcheck: + test: ["CMD", "curl", "-f", "http://localhost:8000/health"] + interval: 30s + timeout: 10s + retries: 3 + restart: unless-stopped
30-31: Enhance volume configuration for production use.The volume configuration should be more specific about the driver and options to use.
volumes: gcs-data: + driver: local + driver_opts: + type: none + device: ${PWD}/data/gcs + o: bind
1-31: Consider adding network isolation and logging configuration.For better security and observability:
- Define a custom network to isolate these services
- Configure logging options
version: "3.8" + +networks: + websocket-net: + driver: bridge + services: websocket: + networks: + - websocket-net + logging: + driver: "json-file" + options: + max-size: "10m" + max-file: "3"websocket/src/auth.rs (1)
58-59: Consider logging or handling unsuccessful HTTP responses more thoroughly.Currently, if the authentication server returns a non-successful status, the method simply returns
Ok(false). It might be beneficial to log the response status or provide more detailed error information for debugging purposes.Apply this diff to log the unsuccessful response:
if !response.status().is_success() { + eprintln!("Authentication failed with status: {}", response.status()); return Ok(false); }websocket/src/conf.rs (1)
49-83: Simplify theDefaultimplementation forConfig.The
Defaulttrait implementation can be simplified by using the existingloadmethod or by refactoring the code to avoid duplication.Consider using the builder pattern or reusing code from the
loadmethod to prevent code duplication.websocket/src/lib.rs (1)
14-14: Add documentation for theAwarenessReftype alias.Providing documentation comments can help other developers understand the purpose of this type alias.
Apply this diff to add documentation:
+/// A thread-safe reference to the `yrs::sync::Awareness` instance. pub type AwarenessRef = Arc<RwLock<yrs::sync::Awareness>>;websocket/src/conf.yaml (1)
1-9: Add configuration documentation and validation.To improve maintainability and prevent configuration errors:
- Add a version field to track configuration schema changes
- Include comments documenting each configuration option
- Consider implementing configuration validation
Example enhanced structure:
version: "1.0" # Configuration schema version # Redis connection settings redis: # Redis connection URL (required) url: redis://redis:6379 # Cache TTL in seconds (default: 3600) ttl: 3600 # Google Cloud Storage settings gcs: # GCS bucket name (required) bucket_name: yrs-dev # GCS endpoint URL (optional, defaults to Google Cloud) endpoint: http://fake-gcs:4443 # Authentication service configuration auth: # Auth service URL (required) url: http://localhost:8080/auth # Request timeout in milliseconds (default: 5000) timeout_ms: 5000websocket/Cargo.toml (2)
13-15: Document the purpose of the "auth" feature.The "auth" feature is enabled by default, but its purpose and functionality are not documented. This makes it difficult for maintainers to understand when to enable or disable this feature.
Consider adding documentation comments above the features section explaining what functionality the "auth" feature provides.
29-29: Remove commented out dependency.The
lmdb-rsdependency is commented out. If it's no longer needed, remove it entirely. If it might be needed later, document why it's kept as a comment.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
websocket/Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (7)
websocket/Cargo.toml(1 hunks)websocket/docker-compose.yml(1 hunks)websocket/src/auth.rs(1 hunks)websocket/src/bin/main.rs(1 hunks)websocket/src/conf.rs(1 hunks)websocket/src/conf.yaml(1 hunks)websocket/src/lib.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- websocket/src/bin/main.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci-websocket / ci
🔇 Additional comments (7)
websocket/docker-compose.yml (1)
9-9: Port configuration inconsistency with benchmark code.The port mapping is still set to 8000:8000, but according to previous review comments, this should be updated to 8080 to align with existing benchmark code.
- - "8000:8000" + - "8080:8080"websocket/src/auth.rs (1)
66-69: Ensure consistent authentication behavior when theauthfeature is disabled.Returning
Ok(true)when authentication is disabled means all tokens are considered valid. Verify that this is the intended behavior and that it does not introduce security risks.If this is not intended, you may want to adjust the logic accordingly.
websocket/src/lib.rs (1)
4-5: Ensure conditional compilation aligns with feature flags.The
authmodule is included based on theauthfeature flag. Verify that this is correctly set in yourCargo.tomland build configurations.Run the following script to check the feature flags:
✅ Verification successful
Feature flag 'auth' is correctly configured and used
The
authfeature flag is properly declared inCargo.tomland consistently used across the codebase. It's included in the default features, ensuring the authentication module is available by default.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the 'auth' feature flag is set correctly. # List all features in Cargo.toml grep '\[features\]' -A 5 Cargo.toml # Check if 'auth' is included grep 'auth' Cargo.tomlLength of output: 148
Script:
#!/bin/bash # Find all Cargo.toml files and examine their contents echo "=== Finding Cargo.toml files ===" fd Cargo.toml echo -e "\n=== Examining Cargo.toml contents ===" fd Cargo.toml --exec cat {} echo -e "\n=== Searching for auth feature references ===" rg -i "feature.*auth|auth.*feature" --type rustLength of output: 33759
websocket/Cargo.toml (4)
52-58: LGTM! Dev dependencies are well structured.The development dependencies include appropriate tools for testing (mockall), benchmarking (criterion), and WebSocket testing (tokio-tungstenite).
60-62: LGTM! Benchmark configuration is properly set up.The benchmark configuration is correctly structured with harness disabled, which is appropriate for criterion benchmarks.
1-62: Fix TOML formatting.The file needs to be formatted according to taplo standards. Consider using
taplo fmtto fix the formatting issues.
20-50: Consider using cargo-audit for dependency security.The project includes several external dependencies. It's important to regularly audit these dependencies for known security vulnerabilities.
Run the following script to check for known vulnerabilities:
✅ Verification successful
Valid security recommendation for dependency auditing
The suggestion to use cargo-audit is a valuable security practice that should be implemented in your development and CI/CD pipeline. While we cannot verify the current security status of dependencies in this environment, regularly running
cargo auditwill help identify known vulnerabilities in your dependencies.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Install and run cargo-audit cargo install cargo-audit cargo auditLength of output: 233
kasugamirai
force-pushed
the
feat/websocket-yrs
branch
from
6007ea5 to
82f5e47
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
websocket/docker-compose.yml (2)
17-20:⚠️ Potential issueRedis configuration needs security enhancements.
The Redis configuration lacks essential security and reliability features.
The previous review comment about adding password protection, volume persistence, and resource limits is still applicable.
22-28:⚠️ Potential issueSecurity improvements needed for fake-gcs configuration.
The current configuration has several security concerns:
- Using unversioned image poses security risks
- HTTP scheme on port 4443 (typically HTTPS)
- Missing security context for the container
Add the following security enhancements:
fake-gcs: - image: fsouza/fake-gcs-server + image: fsouza/fake-gcs-server:v1.47.2 ports: - "4443:4443" - command: ["-scheme", "http"] + command: ["-scheme", "https", "-public-host", "localhost:4443"] volumes: - gcs-data:/data + security_opt: + - no-new-privileges:true + read_only: true + tmpfs: + - /tmpwebsocket/src/bin/main.rs (2)
56-60: 🛠️ Refactor suggestionImprove error handling in bucket creation.
The error handling in
ensure_bucketrelies on string matching, which is fragile. Consider using specific error codes or patterns provided by the GCS client library.match client.insert_bucket(&request).await { Ok(_) => Ok(()), - Err(e) if e.to_string().contains("already exists") => Ok(()), + Err(e) if is_bucket_exists_error(&e) => Ok(()), Err(e) => Err(e.into()), } +fn is_bucket_exists_error(error: &anyhow::Error) -> bool { + if let Some(api_error) = error.downcast_ref::<google_cloud_storage::Error>() { + match api_error { + google_cloud_storage::Error::BucketAlreadyExists(_) => true, + _ => false, + } + } else { + false + } +}
173-176:⚠️ Potential issueHandle potential binding errors when starting the server.
The
unwrap()calls on thebindandservemethods could cause the application to panic if binding to the address fails.-let listener = tokio::net::TcpListener::bind(format!("0.0.0.0:{}", PORT)) - .await - .unwrap(); -axum::serve(listener, app).await.unwrap(); +let listener = match tokio::net::TcpListener::bind(format!("0.0.0.0:{}", PORT)).await { + Ok(l) => l, + Err(e) => { + tracing::error!("Failed to bind to address: {}", e); + return; + } +}; +if let Err(e) = axum::serve(listener, app).await { + tracing::error!("Server error: {}", e); +}websocket/src/broadcast.rs (1)
213-215:⚠️ Potential issueHandle potential errors when processing updates in storage task.
In the storage processing task, errors from
handle_updateare not handled.tokio::spawn(async move { while let Some(update) = rx.recv().await { - Self::handle_update(update, &doc_name, &store, &redis, redis_ttl).await; + if let Err(e) = Self::handle_update(update, &doc_name, &store, &redis, redis_ttl).await { + tracing::error!("Failed to handle update: {}", e); + } } });
🧹 Nitpick comments (11)
websocket/build.sh (1)
6-6: Verify executable permissions.The script should verify that the executable permissions were set correctly.
-chmod +x output/* +echo "Setting executable permissions..." +if ! chmod +x output/*; then + echo "Warning: Failed to set executable permissions" + exit 1 +fi + +echo "Build completed successfully!"websocket/docker-compose.yml (2)
11-15: Enhance service configuration for production readiness.Consider adding:
- Health check configuration for better orchestration
- Resource limits to prevent resource exhaustion
- Restart policy for improved reliability
environment: - REDIS_URL=redis://redis:6379 - GCS_ENDPOINT=http://fake-gcs:4443 + deploy: + resources: + limits: + memory: 512M + cpus: '1.0' + healthcheck: + test: ["CMD", "curl", "-f", "http://localhost:8000/health"] + interval: 30s + timeout: 10s + retries: 3 + restart: unless-stopped depends_on: - redis - fake-gcs
30-31: Enhance volume configuration for data persistence.The volume configuration could be improved for better data management and backup capabilities.
volumes: gcs-data: + driver: local + driver_opts: + type: none + device: ${PWD}/data/gcs + o: bindwebsocket/Cargo.toml (3)
13-15: Document the purpose of the "auth" feature.Please add documentation explaining what functionality the "auth" feature enables and when it should be used.
29-29: Remove commented out dependency.Remove the commented out
lmdb-rsdependency to keep the manifest clean.
39-45: Clarify the database strategy.The project includes multiple database dependencies (Redis, SQLite) without clear separation of concerns. Consider:
- Documenting the purpose of each database
- Using feature flags to make these dependencies optional
- Implementing a clear persistence abstraction layer
websocket/benches/readme.adoc (2)
83-90: Enhance configuration documentation.Please add:
- Hardware requirements/recommendations
- Environment setup instructions
- Expected resource utilization
- System prerequisites (e.g., available ports, required services)
107-111: Add operational details and troubleshooting guide.Please enhance the notes section with:
- Troubleshooting common issues
- Error messages interpretation
- Clean-up procedures
- Resource cleanup between benchmark runs
websocket/benches/server_bench.rs (3)
14-77: Helper functions are well-implemented!Consider enhancing error messages with more context (e.g., include document ID, operation type).
79-180: Consider parameterizing benchmark configurations.Extract magic numbers into constants or configuration parameters:
- Number of text operations
- Delay durations
- Message sizes
This would make the benchmarks more flexible and reusable.
316-393: Add resource monitoring for large operations.Consider adding:
- Memory usage tracking
- Network bandwidth monitoring
- System resource utilization metrics
This would help identify potential bottlenecks and resource constraints.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
websocket/.DS_Storeis excluded by!**/.DS_Storewebsocket/Cargo.lockis excluded by!**/*.lock,!**/*.lockwebsocket/README.mdis excluded by!**/*.md
📒 Files selected for processing (82)
websocket/.dockerignore(0 hunks)websocket/.gitignore(1 hunks)websocket/.taplo.toml(0 hunks)websocket/Cargo.toml(1 hunks)websocket/Dockerfile(1 hunks)websocket/LICENSE(1 hunks)websocket/Makefile.toml(0 hunks)websocket/README.adoc(1 hunks)websocket/Redis.Dockerfile(0 hunks)websocket/Websocket.Dockerfile(0 hunks)websocket/app/Cargo.toml(0 hunks)websocket/app/examples/readme.adoc(0 hunks)websocket/app/examples/room_client.rs(0 hunks)websocket/app/examples/session_client.rs(0 hunks)websocket/app/readme.adoc(0 hunks)websocket/app/src/bin/main.rs(0 hunks)websocket/app/src/config.rs(0 hunks)websocket/app/src/errors.rs(0 hunks)websocket/app/src/handlers/cleanup.rs(0 hunks)websocket/app/src/handlers/heartbeat.rs(0 hunks)websocket/app/src/handlers/message_handler.rs(0 hunks)websocket/app/src/handlers/mod.rs(0 hunks)websocket/app/src/handlers/room_handler.rs(0 hunks)websocket/app/src/handlers/socket_handler.rs(0 hunks)websocket/app/src/handlers/types.rs(0 hunks)websocket/app/src/lib.rs(0 hunks)websocket/app/src/middleware/mod.rs(0 hunks)websocket/app/src/middleware/trace.rs(0 hunks)websocket/app/src/room.rs(0 hunks)websocket/app/src/routes.rs(0 hunks)websocket/app/src/state.rs(0 hunks)websocket/benches/readme.adoc(1 hunks)websocket/benches/server_bench.rs(1 hunks)websocket/build.sh(1 hunks)websocket/clippy.toml(0 hunks)websocket/conf/conf.yaml(0 hunks)websocket/config/config.toml(0 hunks)websocket/crates/infra/Cargo.toml(0 hunks)websocket/crates/infra/src/lib.rs(0 hunks)websocket/crates/infra/src/persistence/editing_session.rs(0 hunks)websocket/crates/infra/src/persistence/event_handler.rs(0 hunks)websocket/crates/infra/src/persistence/gcs/gcs_client.rs(0 hunks)websocket/crates/infra/src/persistence/gcs/mod.rs(0 hunks)websocket/crates/infra/src/persistence/local_storage/local_client.rs(0 hunks)websocket/crates/infra/src/persistence/local_storage/mod.rs(0 hunks)websocket/crates/infra/src/persistence/mod.rs(0 hunks)websocket/crates/infra/src/persistence/project_repository.rs(0 hunks)websocket/crates/infra/src/persistence/redis/connection.rs(0 hunks)websocket/crates/infra/src/persistence/redis/default_key_manager.rs(0 hunks)websocket/crates/infra/src/persistence/redis/errors.rs(0 hunks)websocket/crates/infra/src/persistence/redis/flow_project_lock.rs(0 hunks)websocket/crates/infra/src/persistence/redis/flow_project_redis_data_manager.rs(0 hunks)websocket/crates/infra/src/persistence/redis/keys.rs(0 hunks)websocket/crates/infra/src/persistence/redis/mod.rs(0 hunks)websocket/crates/infra/src/persistence/redis/types.rs(0 hunks)websocket/crates/infra/src/persistence/redis/updates.rs(0 hunks)websocket/crates/infra/src/persistence/repository.rs(0 hunks)websocket/crates/infra/src/types/mod.rs(0 hunks)websocket/crates/infra/src/types/project.rs(0 hunks)websocket/crates/infra/src/types/snapshot.rs(0 hunks)websocket/crates/infra/src/types/user.rs(0 hunks)websocket/crates/infra/src/types/workspace.rs(0 hunks)websocket/crates/infra/src/utils.rs(0 hunks)websocket/crates/services/Cargo.toml(0 hunks)websocket/crates/services/examples/edit_session_service.rs(0 hunks)websocket/crates/services/examples/project_service.rs(0 hunks)websocket/crates/services/examples/readme.adoc(0 hunks)websocket/crates/services/src/error.rs(0 hunks)websocket/crates/services/src/lib.rs(0 hunks)websocket/crates/services/src/manage_project_edit_session.rs(0 hunks)websocket/crates/services/src/project.rs(0 hunks)websocket/crates/services/src/types.rs(0 hunks)websocket/docker-compose.yml(1 hunks)websocket/output/conf.yaml(1 hunks)websocket/output/run.sh(1 hunks)websocket/redis.conf(0 hunks)websocket/rustfmt.toml(0 hunks)websocket/script/run.sh(1 hunks)websocket/src/auth.rs(1 hunks)websocket/src/bin/main.rs(1 hunks)websocket/src/broadcast.rs(1 hunks)websocket/src/broadcast_pool.rs(1 hunks)
⛔ Files not processed due to max files limit (11)
- websocket/src/conf.rs
- websocket/src/conf.yaml
- websocket/src/conn.rs
- websocket/src/lib.rs
- websocket/src/storage/gcs/mod.rs
- websocket/src/storage/kv/error.rs
- websocket/src/storage/kv/keys.rs
- websocket/src/storage/kv/mod.rs
- websocket/src/storage/mod.rs
- websocket/src/storage/sqlite/mod.rs
- websocket/src/ws.rs
💤 Files with no reviewable changes (66)
- websocket/app/src/middleware/trace.rs
- websocket/config/config.toml
- websocket/crates/infra/src/persistence/redis/connection.rs
- websocket/.dockerignore
- websocket/app/src/routes.rs
- websocket/Redis.Dockerfile
- websocket/clippy.toml
- websocket/redis.conf
- websocket/crates/infra/src/utils.rs
- websocket/rustfmt.toml
- websocket/crates/infra/src/lib.rs
- websocket/.taplo.toml
- websocket/crates/services/src/project.rs
- websocket/crates/services/examples/readme.adoc
- websocket/crates/infra/src/persistence/gcs/mod.rs
- websocket/app/readme.adoc
- websocket/crates/infra/Cargo.toml
- websocket/crates/infra/src/persistence/editing_session.rs
- websocket/app/src/bin/main.rs
- websocket/app/examples/readme.adoc
- websocket/crates/infra/src/persistence/event_handler.rs
- websocket/Websocket.Dockerfile
- websocket/app/Cargo.toml
- websocket/app/src/handlers/heartbeat.rs
- websocket/app/examples/room_client.rs
- websocket/crates/services/examples/project_service.rs
- websocket/crates/infra/src/persistence/local_storage/mod.rs
- websocket/crates/infra/src/persistence/redis/default_key_manager.rs
- websocket/crates/infra/src/types/project.rs
- websocket/app/src/middleware/mod.rs
- websocket/crates/infra/src/persistence/project_repository.rs
- websocket/app/src/errors.rs
- websocket/crates/infra/src/persistence/repository.rs
- websocket/crates/services/src/error.rs
- websocket/app/src/handlers/cleanup.rs
- websocket/crates/services/src/types.rs
- websocket/crates/infra/src/persistence/redis/types.rs
- websocket/app/src/handlers/types.rs
- websocket/crates/infra/src/types/mod.rs
- websocket/app/src/handlers/room_handler.rs
- websocket/app/src/handlers/mod.rs
- websocket/crates/infra/src/persistence/mod.rs
- websocket/app/src/lib.rs
- websocket/crates/infra/src/persistence/redis/mod.rs
- websocket/app/src/handlers/socket_handler.rs
- websocket/conf/conf.yaml
- websocket/crates/services/examples/edit_session_service.rs
- websocket/app/src/state.rs
- websocket/crates/infra/src/persistence/redis/keys.rs
- websocket/crates/infra/src/persistence/redis/errors.rs
- websocket/crates/services/src/lib.rs
- websocket/Makefile.toml
- websocket/app/src/handlers/message_handler.rs
- websocket/app/src/config.rs
- websocket/crates/infra/src/persistence/redis/flow_project_lock.rs
- websocket/crates/infra/src/types/user.rs
- websocket/app/examples/session_client.rs
- websocket/crates/infra/src/persistence/redis/updates.rs
- websocket/crates/infra/src/types/snapshot.rs
- websocket/crates/infra/src/persistence/redis/flow_project_redis_data_manager.rs
- websocket/crates/services/src/manage_project_edit_session.rs
- websocket/crates/infra/src/persistence/local_storage/local_client.rs
- websocket/crates/services/Cargo.toml
- websocket/app/src/room.rs
- websocket/crates/infra/src/types/workspace.rs
- websocket/crates/infra/src/persistence/gcs/gcs_client.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- websocket/.gitignore
- websocket/LICENSE
- websocket/README.adoc
- websocket/Dockerfile
- websocket/src/broadcast_pool.rs
- websocket/src/auth.rs
🧰 Additional context used
🪛 Shellcheck (0.10.0)
websocket/build.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
websocket/output/run.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
websocket/script/run.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🪛 yamllint (1.35.1)
websocket/output/conf.yaml
[error] 6-6: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (13)
websocket/build.sh (4)
1-6: Add error handling and shell specification.The script needs proper error handling and shell specification to ensure reliable execution.
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
4-5:⚠️ Potential issueAdd verification for successful cargo build.
The script should verify that the cargo build succeeded and the binary exists before proceeding.
-cargo build --bin main --release -cp target/release/main output +echo "Building release binary..." +if ! cargo build --bin main --release; then + echo "Error: Cargo build failed" + exit 1 +fi + +if [ ! -f "target/release/main" ]; then + echo "Error: Binary not found after build" + exit 1 +fi + +echo "Copying binary to output directory..." +cp target/release/main outputLikely invalid or redundant comment.
2-3:⚠️ Potential issueAvoid silently suppressing errors with
2>/dev/null.Silently suppressing errors can mask important issues. Instead, implement proper error checking and provide meaningful feedback.
-cp script/* output 2>/dev/null -cp -r src/conf.yaml output/ 2>/dev/null +# Copy scripts if directory exists +if [ -d "script" ]; then + echo "Copying scripts to output directory..." + cp script/* output || echo "Warning: No scripts to copy" +fi + +# Copy config if exists +if [ -f "src/conf.yaml" ]; then + echo "Copying configuration file..." + cp -r src/conf.yaml output/ +else + echo "Warning: Configuration file not found at src/conf.yaml" +fiLikely invalid or redundant comment.
1-1: 🛠️ Refactor suggestionAdd cleanup of previous builds.
The script should clean up any previous build artifacts to ensure a fresh build.
+# Clean previous build +if [ -d "output" ]; then + echo "Cleaning previous build..." + rm -rf output +fi + +echo "Creating output directory..." mkdir outputLikely invalid or redundant comment.
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
websocket/docker-compose.yml (1)
9-9: Port configuration conflicts with existing benchmark code.The port mapping of 8000:8000 conflicts with the existing benchmark code that expects port 8080.
#!/bin/bash # Search for WebSocket client configurations and port references rg 'ws://(localhost|127\.0\.0\.1):(8000|8080)|new WebSocket.*8000|new WebSocket.*8080'websocket/Cargo.toml (2)
1-11: Update package metadata to match the project context.The current metadata appears to be copied from the
yrs-warpproject without proper adaptation.
52-62: Development dependencies and bench configuration look good!websocket/benches/readme.adoc (1)
26-82: Add performance baselines and complexity analysis.The benchmark descriptions would be more useful with performance expectations and resource requirements.
websocket/benches/server_bench.rs (1)
480-501: Criterion configuration and benchmark organization look great!The configuration provides a good balance between accuracy and execution time.
websocket/script/run.sh (1)
1-1: Enhance script robustness.The run script needs improvements for better reliability:
+#!/bin/bash +set -e + +# Verify binary exists and is executable +if [ ! -x "./main" ]; then + echo "Error: main binary not found or not executable" + exit 1 +fi + ./main🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
websocket/src/broadcast.rs (2)
57-58: Justify unsafe Send/Sync implementations.The manual implementation of
SendandSynctraits usingunsaferequires justification. Please document why these implementations are safe.
386-419: Handle unknown message types inhandle_msg.The
matchstatement inhandle_msgdoes not account for all possible message types. Consider adding an_case to catch unexpected messages.match msg { // Existing cases... Message::Custom(tag, data) => { let awareness = awareness.write().await; protocol.missing_handle(&awareness, tag, data) } + _ => { + tracing::warn!("Received unknown message type"); + Err(Error::Other(Box::new(std::io::Error::new( + std::io::ErrorKind::InvalidData, + "Unknown message type", + )))) + } }websocket/output/run.sh (1)
1-1: Enhance script robustness.The run script needs improvements for better reliability:
+#!/bin/bash +set -e + +# Verify binary exists and is executable +if [ ! -x "./main" ]; then + echo "Error: main binary not found or not executable" + exit 1 +fi + ./main🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
api/internal/app/public_test.go (1)
19-101: Enhance test coverage with additional test cases.While the current test cases are good, consider adding these scenarios for more comprehensive coverage:
- Malformed JSON request
- Invalid Content-Type header
- Token with special characters
Also, consider adding comments to describe each test case's purpose:
tests := []struct { + // Test case fields documentation + // name: descriptive name of the test case + // requestBody: the request payload to send + // setupContext: function to configure the context + // expectedStatus: expected HTTP status code + // expectedAuth: expected authorization result name string requestBody interface{} setupContext func(context.Context) context.Contextapi/internal/app/public.go (1)
106-108: Consider adding validation tags to the AuthRequest struct.Add validation tags to ensure the token meets minimum requirements:
type AuthRequest struct { - Token string `json:"token"` + Token string `json:"token" validate:"required,min=1"` }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/internal/app/app.go(1 hunks)api/internal/app/public.go(1 hunks)api/internal/app/public_test.go(1 hunks)
🔇 Additional comments (2)
api/internal/app/app.go (1)
97-97: Verify alignment with PR objectives.The addition of an auth verification endpoint appears unrelated to the PR's stated objective of updating the websocket server to yrs-warp. Please clarify if this change is necessary for the websocket refactoring or should be in a separate PR.
api/internal/app/public.go (1)
Line range hint
1-1: Verify PR scope and title accuracy.The PR title suggests a websocket server update to yrs-warp, but the changes appear to be focused on adding authentication verification functionality. Please either:
- Update the PR title and description to reflect the actual changes, or
- Clarify how these auth changes relate to the websocket refactoring, or
- Consider splitting these changes into a separate PR if they're unrelated to the websocket updates.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
websocket/src/auth.rs (1)
18-27:⚠️ Potential issueAvoid using
expectto handle errors when building the HTTP client.Using
expecthere will cause a panic if the client cannot be created. It's better to handle the potential error gracefully by returning aResult<Self>.Apply this diff to modify the
newmethod to return aResult<Self>:-pub fn new(config: AuthConfig) -> Self { +pub fn new(config: AuthConfig) -> Result<Self> { { let client = Client::builder() .timeout(Duration::from_millis(config.timeout_ms)) - .build() - .expect("Failed to create HTTP client"); + .build()?; - Self { client, config } + Ok(Self { client, config }) } }websocket/src/bin/main.rs (2)
45-61:⚠️ Potential issueImprove error handling in
ensure_bucketfunction.Relying on error message string matching to check if the bucket already exists is fragile and may break if the error message changes.
173-176:⚠️ Potential issueHandle potential binding errors when starting the server.
The
unwrap()calls on thebindandservemethods could cause the application to panic if binding to the address fails.
🧹 Nitpick comments (5)
websocket/src/auth.rs (2)
7-10: Add documentation for the AuthResponse struct.Consider adding documentation to explain the purpose and usage of this struct, especially since it's part of the authentication flow.
Add this documentation:
+/// Response from the authentication endpoint indicating whether the token is valid. #[derive(Debug, Deserialize, Serialize)] struct AuthResponse { authorized: bool, }
29-45: Add timeout logging for better observability.The verification request has a timeout set, but timeouts aren't explicitly logged. This could help with debugging authentication issues.
pub async fn verify_token(&self, token: &str) -> Result<bool> { { - let response = self + let response = match self .client .get(&self.config.url) .header("Authorization", format!("Bearer {}", token)) .send() - .await?; + .await { + Ok(resp) => resp, + Err(e) if e.is_timeout() => { + tracing::warn!("Authentication request timed out after {}ms", self.config.timeout_ms); + return Ok(false); + } + Err(e) => return Err(e.into()), + };websocket/src/bin/main.rs (3)
82-86: Extract doc_id normalization to a helper function.The doc_id modification logic would be more maintainable and testable as a separate function.
+fn normalize_doc_id(doc_id: String) -> String { + if doc_id.ends_with(":main") { + doc_id[..doc_id.len() - 5].to_string() + } else { + doc_id + } +} async fn ws_handler( ws: WebSocketUpgrade, Path(doc_id): Path<String>, #[cfg(feature = "auth")] Query(auth): Query<AuthQuery>, axum::extract::State(state): axum::extract::State<AppState>, ) -> Response { - let doc_id = if doc_id.ends_with(":main") { - doc_id[..doc_id.len() - 5].to_string() - } else { - doc_id.clone() - }; + let doc_id = normalize_doc_id(doc_id);
107-119: Add connection count logging for better observability.Logging connection counts would help monitor the WebSocket server's usage and potential issues.
async fn handle_socket( socket: WebSocket, bcast: Arc<BroadcastGroup>, doc_id: String, pool: Arc<BroadcastPool>, ) { bcast.increment_connections(); + let count = bcast.connection_count(); + tracing::info!("New connection established for doc_id: {}. Total connections: {}", doc_id, count); let conn = WarpConn::new(bcast, socket); if let Err(e) = conn.await { tracing::error!("WebSocket connection error: {}", e); } pool.remove_connection(&doc_id).await; + tracing::info!("Connection closed for doc_id: {}", doc_id); }
36-36: Make the port configurable through environment variables.Hard-coding the port number reduces flexibility. Consider making it configurable through environment variables or the config file.
-const PORT: &str = "8000"; +const DEFAULT_PORT: &str = "8000"; + +fn get_port() -> String { + std::env::var("WEBSOCKET_PORT").unwrap_or_else(|_| DEFAULT_PORT.to_string()) +}Then update the server binding:
- tracing::info!("Starting server on 0.0.0.0:{}", PORT); - let listener = tokio::net::TcpListener::bind(format!("0.0.0.0:{}", PORT)) + let port = get_port(); + tracing::info!("Starting server on 0.0.0.0:{}", port); + let listener = tokio::net::TcpListener::bind(format!("0.0.0.0:{}", port))
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
websocket/src/bin/main.rs (1)
56-60: 🛠️ Refactor suggestionImprove error handling in bucket creation.
Using string matching for error detection is fragile. Use proper error types or codes provided by the GCS client.
🧹 Nitpick comments (15)
websocket/docker-compose.yml (1)
26-30: Enhance volume configuration for production use.Consider adding production-ready configurations for the GCS data volume:
- Specify a volume driver suitable for production
- Define backup strategy
- Configure volume labels for better management
volumes: gcs-data: + driver: local # Change to appropriate production driver + labels: + backup: "true" + environment: "production"websocket/Cargo.toml (6)
13-15: Document the auth feature flag.Add documentation to explain the purpose and impact of the
authfeature flag. This helps other developers understand when and why they might want to disable authentication.[features] default = ["auth"] -auth = [] +auth = [] # Enable authentication support for the WebSocket server
30-30: Remove commented dependency.Remove the commented out
lmdb-rsdependency. If it's needed in the future, it can be added back through version control.-# lmdb-rs = { version = "0.7" }
40-40: Review database architecture decisions.The project includes multiple database dependencies:
- Google Cloud Storage
- Redis
- SQLite (with connection pooling)
Consider:
- Documenting the purpose of each storage backend
- Making these dependencies optional through feature flags
- Implementing a common interface for storage operations
Would you like me to help create feature flags for these storage backends and design a common storage interface?
Also applies to: 43-46
24-26: Use consistent version constraints.Some dependencies specify exact versions while others use minimum versions. Consider:
- Using minimum versions (
version = ">=1.43") for better compatibility- Or using exact versions (
version = "=1.43") for reproducible buildsExample dependencies to update:
-tokio = { version = "1.43", features = ["full"] } -serde = { version = "1.0.217", features = ["derive", "rc"] } -serde_json = "1.0.137" +tokio = { version = ">=1.43", features = ["full"] } +serde = { version = ">=1.0.217", features = ["derive", "rc"] } +serde_json = ">=1.0.137"Also applies to: 50-50
54-54: Remove duplicate tokio dependency.tokio is already specified in the main dependencies. Remove it from dev-dependencies to avoid potential version conflicts.
[dev-dependencies] tokio-tungstenite = "0.24.0" -tokio = { version = "1", features = ["full"] } bytes = "1.6"
60-62: Document the benchmark configuration.Add a comment explaining the purpose of the benchmark and why the harness is disabled.
+# Benchmark for measuring WebSocket server performance +# Harness is disabled to use criterion's custom benchmarking framework [[bench]] name = "server_bench" harness = falsewebsocket/src/conf.rs (4)
21-31: Add documentation for configuration structs.The
AuthConfigandAppConfigstructs would benefit from documentation comments explaining their purpose and field requirements.Add documentation like this:
+/// Configuration for authentication settings #[derive(Debug, Deserialize)] pub struct AuthConfig { + /// URL of the authentication service pub url: String, + /// Timeout in milliseconds for authentication requests pub timeout_ms: u64, } +/// Application-wide configuration settings #[derive(Debug, Deserialize)] pub struct AppConfig { + /// Environment name (e.g., "development", "production") pub env: String, + /// Comma-separated list of allowed CORS origins pub origins: String, }
21-40: Add validation for URL and timeout fields.The configuration structs should validate their fields to ensure they contain valid values.
Consider implementing validation like this:
use url::Url; impl AuthConfig { fn validate(&self) -> Result<(), String> { // Validate URL Url::parse(&self.url) .map_err(|e| format!("Invalid auth URL: {}", e))?; // Validate timeout (e.g., not too short or too long) if self.timeout_ms < 1000 || self.timeout_ms > 30000 { return Err("Timeout must be between 1000 and 30000 ms".to_string()); } Ok(()) } } impl AppConfig { fn validate(&self) -> Result<(), String> { // Validate environment match self.env.as_str() { "development" | "production" | "staging" => Ok(()), _ => Err("Invalid environment".to_string()), }?; // Validate origins for origin in self.origins.split(',') { Url::parse(origin.trim()) .map_err(|e| format!("Invalid origin URL {}: {}", origin, e))?; } Ok(()) } }
64-94: Refactor repetitive environment variable loading.The current implementation has repetitive patterns for loading environment variables. Consider using a macro or helper function to reduce duplication.
Consider implementing a helper like this:
impl Config { fn load_env_var<T: std::str::FromStr>(key: &str, default: T) -> T { env::var(key) .ok() .and_then(|v| v.parse().ok()) .unwrap_or(default) } }Then use it like:
- if let Ok(url) = env::var("REEARTH_FLOW_REDIS_URL") { - config.redis.url = url; - } + config.redis.url = Self::load_env_var("REEARTH_FLOW_REDIS_URL", + DEFAULT_REDIS_URL.to_string());
109-149: Reduce code duplication in Default implementation.The current implementation duplicates configuration values between feature-gated variants. Consider extracting common configuration into a separate function.
Refactor like this:
impl Default for Config { fn default() -> Self { let base_config = |config: &mut Config| { config.redis = RedisConfig { url: DEFAULT_REDIS_URL.to_string(), ttl: DEFAULT_REDIS_TTL, }; config.gcs = GcsConfig { bucket_name: DEFAULT_GCS_BUCKET.to_string(), endpoint: Some(DEFAULT_GCS_ENDPOINT.to_string()), }; config.app = AppConfig { env: DEFAULT_APP_ENV.to_string(), origins: DEFAULT_ORIGINS.to_string(), }; }; #[cfg(feature = "auth")] { let mut config = Config { redis: RedisConfig::default(), gcs: GcsConfig::default(), auth: AuthConfig { url: DEFAULT_AUTH_URL.to_string(), timeout_ms: DEFAULT_AUTH_TIMEOUT_MS, }, app: AppConfig::default(), }; base_config(&mut config); config } #[cfg(not(feature = "auth"))] { let mut config = Config { redis: RedisConfig::default(), gcs: GcsConfig::default(), app: AppConfig::default(), }; base_config(&mut config); config } } }websocket/src/bin/main.rs (4)
32-34: Remove commented-out code.These commented configuration constants are not being used. If they're no longer needed, they should be removed to maintain code cleanliness.
38-43: Enhance token validation requirements.The
AuthQuerystruct accepts any string as a token without format validation. Consider adding validation for token format and minimum strength requirements.#[cfg(feature = "auth")] #[derive(Debug, Deserialize)] struct AuthQuery { #[serde(default)] + #[serde(deserialize_with = "validate_token")] token: String, } +fn validate_token<'de, D>(deserializer: D) -> Result<String, D::Error> +where + D: serde::Deserializer<'de>, +{ + let token = String::deserialize(deserializer)?; + if token.len() < 32 { + return Err(serde::de::Error::custom("Token too short")); + } + // Add more validation as needed + Ok(token) +}
82-86: Improve document ID handling.The document ID manipulation logic should be moved to a separate function for better testability and reuse. Also, consider using a constant for the ":main" suffix.
+const MAIN_SUFFIX: &str = ":main"; +fn normalize_doc_id(doc_id: &str) -> String { + if doc_id.ends_with(MAIN_SUFFIX) { + doc_id[..doc_id.len() - MAIN_SUFFIX.len()].to_string() + } else { + doc_id.to_string() + } +} async fn ws_handler( // ... params ... ) -> Response { - let doc_id = if doc_id.ends_with(":main") { - doc_id[..doc_id.len() - 5].to_string() - } else { - doc_id.clone() - }; + let doc_id = normalize_doc_id(&doc_id);
132-134: Remove commented SQLite initialization code.Remove commented-out code to improve maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
websocket/Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (7)
api/internal/app/app.go(1 hunks)websocket/.env-sample(1 hunks)websocket/Cargo.toml(1 hunks)websocket/Dockerfile(1 hunks)websocket/docker-compose.yml(1 hunks)websocket/src/bin/main.rs(1 hunks)websocket/src/conf.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- websocket/.env-sample
🚧 Files skipped from review as they are similar to previous changes (2)
- api/internal/app/app.go
- websocket/Dockerfile
🔇 Additional comments (7)
websocket/docker-compose.yml (3)
9-9: Update WebSocket port to 8080 for consistency.The port should be changed from 8000 to 8080 to align with existing benchmark code and documentation updates.
- - "8000:8000" + - "8080:8080"
16-19: Enhance Redis configuration for production readiness.The Redis configuration needs essential security and reliability improvements.
redis: image: redis:7-alpine ports: - "6379:6379" + command: redis-server --requirepass ${REDIS_PASSWORD} + environment: + - REDIS_PASSWORD=${REDIS_PASSWORD} + volumes: + - redis-data:/data + deploy: + resources: + limits: + memory: 1G
21-25: Address security and versioning concerns in fake-gcs configuration.The configuration needs improvements for security and stability.
fake-gcs: - image: fsouza/fake-gcs-server + image: fsouza/fake-gcs-server:v1.47.2 ports: - "4443:4443" - command: ["-scheme", "http"] + command: ["-scheme", "https", "-public-host", "localhost:4443"]websocket/Cargo.toml (1)
1-11: Update package metadata to match the project context.The metadata appears to be copied from the
yrs-warpproject without proper adaptation:
- Homepage and repository URLs should point to
reearth/reearth-flow- Authors should reflect the Re:Earth Flow Contributors
- Keywords might need adjustment to match the project's context
Apply this diff to align the metadata with the project:
[package] name = "websocket" version = "0.1.0" edition = "2021" description = "Yrs synchronization protocol using Warp web sockets" license = "MIT" -authors = ["Bartosz Sypytkowski <[email protected]>"] -keywords = ["crdt", "yrs", "warp"] -homepage = "https://github.com/y-crdt/yrs-warp/" -repository = "https://github.com/y-crdt/yrs-warp/" +authors = ["Re:Earth Flow Contributors"] +keywords = ["crdt", "yrs", "warp", "reearth"] +homepage = "https://github.com/reearth/reearth-flow" +repository = "https://github.com/reearth/reearth-flow" readme = "./README.md"websocket/src/conf.rs (1)
1-20: LGTM! Well-organized imports and constants.The imports are appropriate, and the constants are well-structured with reasonable default values for local development.
websocket/src/bin/main.rs (2)
170-174:⚠️ Potential issueImprove error handling in server initialization.
Using
unwrap()for error handling in server initialization could cause panic. Implement proper error handling.- let listener = tokio::net::TcpListener::bind(format!("0.0.0.0:{}", PORT)) - .await - .unwrap(); - axum::serve(listener, app).await.unwrap(); + let addr = format!("0.0.0.0:{}", PORT); + let listener = match tokio::net::TcpListener::bind(&addr).await { + Ok(l) => l, + Err(e) => { + tracing::error!("Failed to bind to {}: {}", addr, e); + std::process::exit(1); + } + }; + if let Err(e) = axum::serve(listener, app).await { + tracing::error!("Server error: {}", e); + std::process::exit(1); + }Likely invalid or redundant comment.
115-119: Potential race condition in connection cleanup.The connection cleanup in
handle_socketmight have a race condition if multiple connections for the same document are being closed simultaneously. Consider using atomic operations or proper synchronization.
kasugamirai
force-pushed
the
feat/websocket-yrs
branch
from
4211aa2 to
1031c87
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
websocket/docker-compose.yml (2)
4-15: Add health check and resource limits for production readiness.The websocket service configuration could be improved for production use:
- Add healthcheck to ensure service availability
- Set resource limits to prevent resource exhaustion
- Consider explicitly defining critical environment variables
websocket: build: context: . dockerfile: Dockerfile ports: - "8080:8080" env_file: - .env + environment: + - REDIS_HOST=redis + - GCS_ENDPOINT=http://fake-gcs:4443 + healthcheck: + test: ["CMD", "curl", "-f", "http://localhost:8080/health"] + interval: 30s + timeout: 10s + retries: 3 + deploy: + resources: + limits: + memory: 1G + cpus: '1' depends_on: - redis - fake-gcs
29-30: Enhance volume configuration with explicit settings.The volume configuration could be improved with explicit driver and options settings for better control and maintenance.
volumes: gcs-data: + driver: local + driver_opts: + type: none + device: ${PWD}/data/gcs + o: bindwebsocket/benches/readme.adoc (2)
9-25: Add prerequisites and system requirements.Consider adding the following information to help users prepare for and run the benchmarks effectively:
- System requirements (CPU, memory, disk space)
- Required dependencies and their versions
- Expected duration of the complete benchmark suite
- Environment setup instructions
83-111: Enhance configuration and troubleshooting sections.Consider adding:
Instructions for customizing benchmark parameters:
- How to modify sample sizes
- How to adjust delays
- How to target specific benchmarks
Troubleshooting guide:
- Common issues and solutions
- How to handle failed benchmarks
- Performance anomaly investigation
Best practices:
- System preparation for consistent results
- When to run benchmarks (e.g., system load considerations)
- How to compare results across different runs
ui/src/lib/yjs/useYjsSetup.ts (2)
6-6: Reorder imports to follow conventions.Move the
@flow/lib/authimport after@flow/global-constantsto maintain consistent import ordering.-import { useAuth } from "@flow/lib/auth"; import { config } from "@flow/config"; import { DEFAULT_ENTRY_GRAPH_ID } from "@flow/global-constants"; +import { useAuth } from "@flow/lib/auth";🧰 Tools
🪛 GitHub Check: ci-ui / ci
[warning] 6-6:
@flow/lib/authimport should occur after import of@flow/global-constants
35-61: Add error handling for token retrieval.The async token retrieval should include error handling to gracefully handle authentication failures.
(async () => { + try { const token = await getAccessToken(); yWebSocketProvider = new WebsocketProvider( websocket, `${projectId}:${workflowId}`, yDoc, { params: { token, }, }, ); yWebSocketProvider.once("sync", () => { if (yWorkflows.length === 0) { yDoc.transact(() => { const yWorkflow = yWorkflowBuilder( DEFAULT_ENTRY_GRAPH_ID, "Main Workflow", ); yWorkflows.push([yWorkflow]); }); } setIsSynced(true); }); + } catch (error) { + console.error('Failed to initialize WebSocket connection:', error); + // Consider adding user notification or retry logic + } })();websocket/Cargo.toml (1)
13-15: Consider expanding feature flags for optional dependenciesThe auth feature is a good start, but consider making external services optional as well.
[features] -default = ["auth"] -auth = [] +default = ["auth", "redis-storage", "gcs-storage"] +auth = [] +redis-storage = ["redis"] +gcs-storage = ["google-cloud-storage"]
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
websocket/.DS_Storeis excluded by!**/.DS_Storewebsocket/Cargo.lockis excluded by!**/*.lock,!**/*.lockwebsocket/README.mdis excluded by!**/*.md
📒 Files selected for processing (82)
api/internal/app/app.go(2 hunks)ui/src/lib/yjs/useYjsSetup.ts(4 hunks)websocket/.dockerignore(0 hunks)websocket/.env-sample(1 hunks)websocket/.gitignore(1 hunks)websocket/.taplo.toml(0 hunks)websocket/Cargo.toml(1 hunks)websocket/Dockerfile(1 hunks)websocket/LICENSE(1 hunks)websocket/Makefile.toml(0 hunks)websocket/README.adoc(1 hunks)websocket/Redis.Dockerfile(0 hunks)websocket/Websocket.Dockerfile(0 hunks)websocket/app/Cargo.toml(0 hunks)websocket/app/examples/readme.adoc(0 hunks)websocket/app/examples/room_client.rs(0 hunks)websocket/app/examples/session_client.rs(0 hunks)websocket/app/readme.adoc(0 hunks)websocket/app/src/bin/main.rs(0 hunks)websocket/app/src/config.rs(0 hunks)websocket/app/src/errors.rs(0 hunks)websocket/app/src/handlers/cleanup.rs(0 hunks)websocket/app/src/handlers/heartbeat.rs(0 hunks)websocket/app/src/handlers/message_handler.rs(0 hunks)websocket/app/src/handlers/mod.rs(0 hunks)websocket/app/src/handlers/room_handler.rs(0 hunks)websocket/app/src/handlers/socket_handler.rs(0 hunks)websocket/app/src/handlers/types.rs(0 hunks)websocket/app/src/lib.rs(0 hunks)websocket/app/src/middleware/mod.rs(0 hunks)websocket/app/src/middleware/trace.rs(0 hunks)websocket/app/src/room.rs(0 hunks)websocket/app/src/routes.rs(0 hunks)websocket/app/src/state.rs(0 hunks)websocket/benches/readme.adoc(1 hunks)websocket/benches/server_bench.rs(1 hunks)websocket/build.sh(1 hunks)websocket/clippy.toml(0 hunks)websocket/conf/conf.yaml(0 hunks)websocket/config/config.toml(0 hunks)websocket/crates/infra/Cargo.toml(0 hunks)websocket/crates/infra/src/lib.rs(0 hunks)websocket/crates/infra/src/persistence/editing_session.rs(0 hunks)websocket/crates/infra/src/persistence/event_handler.rs(0 hunks)websocket/crates/infra/src/persistence/gcs/gcs_client.rs(0 hunks)websocket/crates/infra/src/persistence/gcs/mod.rs(0 hunks)websocket/crates/infra/src/persistence/local_storage/local_client.rs(0 hunks)websocket/crates/infra/src/persistence/local_storage/mod.rs(0 hunks)websocket/crates/infra/src/persistence/mod.rs(0 hunks)websocket/crates/infra/src/persistence/project_repository.rs(0 hunks)websocket/crates/infra/src/persistence/redis/connection.rs(0 hunks)websocket/crates/infra/src/persistence/redis/default_key_manager.rs(0 hunks)websocket/crates/infra/src/persistence/redis/errors.rs(0 hunks)websocket/crates/infra/src/persistence/redis/flow_project_lock.rs(0 hunks)websocket/crates/infra/src/persistence/redis/flow_project_redis_data_manager.rs(0 hunks)websocket/crates/infra/src/persistence/redis/keys.rs(0 hunks)websocket/crates/infra/src/persistence/redis/mod.rs(0 hunks)websocket/crates/infra/src/persistence/redis/types.rs(0 hunks)websocket/crates/infra/src/persistence/redis/updates.rs(0 hunks)websocket/crates/infra/src/persistence/repository.rs(0 hunks)websocket/crates/infra/src/types/mod.rs(0 hunks)websocket/crates/infra/src/types/project.rs(0 hunks)websocket/crates/infra/src/types/snapshot.rs(0 hunks)websocket/crates/infra/src/types/user.rs(0 hunks)websocket/crates/infra/src/types/workspace.rs(0 hunks)websocket/crates/infra/src/utils.rs(0 hunks)websocket/crates/services/Cargo.toml(0 hunks)websocket/crates/services/examples/edit_session_service.rs(0 hunks)websocket/crates/services/examples/project_service.rs(0 hunks)websocket/crates/services/examples/readme.adoc(0 hunks)websocket/crates/services/src/error.rs(0 hunks)websocket/crates/services/src/lib.rs(0 hunks)websocket/crates/services/src/manage_project_edit_session.rs(0 hunks)websocket/crates/services/src/project.rs(0 hunks)websocket/crates/services/src/types.rs(0 hunks)websocket/docker-compose.yml(1 hunks)websocket/output/conf.yaml(1 hunks)websocket/output/run.sh(1 hunks)websocket/redis.conf(0 hunks)websocket/rustfmt.toml(0 hunks)websocket/script/run.sh(1 hunks)websocket/src/auth.rs(1 hunks)
⛔ Files not processed due to max files limit (13)
- websocket/src/bin/main.rs
- websocket/src/broadcast.rs
- websocket/src/broadcast_pool.rs
- websocket/src/conf.rs
- websocket/src/conn.rs
- websocket/src/lib.rs
- websocket/src/storage/gcs/mod.rs
- websocket/src/storage/kv/error.rs
- websocket/src/storage/kv/keys.rs
- websocket/src/storage/kv/mod.rs
- websocket/src/storage/mod.rs
- websocket/src/storage/sqlite/mod.rs
- websocket/src/ws.rs
💤 Files with no reviewable changes (66)
- websocket/clippy.toml
- websocket/app/src/middleware/trace.rs
- websocket/crates/services/examples/readme.adoc
- websocket/crates/infra/src/utils.rs
- websocket/redis.conf
- websocket/.taplo.toml
- websocket/Redis.Dockerfile
- websocket/.dockerignore
- websocket/crates/infra/src/persistence/gcs/mod.rs
- websocket/app/Cargo.toml
- websocket/app/readme.adoc
- websocket/rustfmt.toml
- websocket/crates/services/src/project.rs
- websocket/app/examples/readme.adoc
- websocket/config/config.toml
- websocket/conf/conf.yaml
- websocket/app/src/bin/main.rs
- websocket/crates/infra/Cargo.toml
- websocket/app/src/routes.rs
- websocket/app/src/handlers/mod.rs
- websocket/app/src/errors.rs
- websocket/crates/infra/src/persistence/local_storage/mod.rs
- websocket/crates/infra/src/persistence/editing_session.rs
- websocket/crates/infra/src/types/mod.rs
- websocket/crates/services/examples/edit_session_service.rs
- websocket/crates/services/src/types.rs
- websocket/crates/infra/src/persistence/redis/connection.rs
- websocket/app/src/handlers/types.rs
- websocket/crates/infra/src/lib.rs
- websocket/crates/infra/src/persistence/event_handler.rs
- websocket/Websocket.Dockerfile
- websocket/crates/services/src/lib.rs
- websocket/crates/infra/src/persistence/redis/default_key_manager.rs
- websocket/crates/infra/src/persistence/redis/types.rs
- websocket/crates/infra/src/persistence/redis/keys.rs
- websocket/crates/infra/src/persistence/redis/errors.rs
- websocket/app/src/handlers/heartbeat.rs
- websocket/Makefile.toml
- websocket/app/src/handlers/message_handler.rs
- websocket/crates/infra/src/persistence/mod.rs
- websocket/crates/infra/src/persistence/project_repository.rs
- websocket/app/examples/room_client.rs
- websocket/app/src/lib.rs
- websocket/crates/infra/src/types/user.rs
- websocket/crates/infra/src/types/project.rs
- websocket/app/src/config.rs
- websocket/app/src/handlers/cleanup.rs
- websocket/crates/infra/src/persistence/repository.rs
- websocket/crates/services/src/error.rs
- websocket/crates/services/examples/project_service.rs
- websocket/app/examples/session_client.rs
- websocket/crates/infra/src/persistence/redis/mod.rs
- websocket/app/src/state.rs
- websocket/crates/infra/src/persistence/redis/flow_project_redis_data_manager.rs
- websocket/app/src/handlers/room_handler.rs
- websocket/crates/infra/src/persistence/redis/flow_project_lock.rs
- websocket/app/src/middleware/mod.rs
- websocket/app/src/room.rs
- websocket/crates/infra/src/types/snapshot.rs
- websocket/app/src/handlers/socket_handler.rs
- websocket/crates/services/Cargo.toml
- websocket/crates/infra/src/persistence/redis/updates.rs
- websocket/crates/infra/src/persistence/local_storage/local_client.rs
- websocket/crates/services/src/manage_project_edit_session.rs
- websocket/crates/infra/src/persistence/gcs/gcs_client.rs
- websocket/crates/infra/src/types/workspace.rs
✅ Files skipped from review due to trivial changes (1)
- websocket/.env-sample
🚧 Files skipped from review as they are similar to previous changes (7)
- websocket/LICENSE
- websocket/.gitignore
- websocket/Dockerfile
- websocket/README.adoc
- api/internal/app/app.go
- websocket/src/auth.rs
- websocket/benches/server_bench.rs
🧰 Additional context used
🪛 GitHub Check: ci-ui / ci
ui/src/lib/yjs/useYjsSetup.ts
[warning] 6-6:
@flow/lib/auth import should occur after import of @flow/global-constants
🪛 Shellcheck (0.10.0)
websocket/build.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
websocket/output/run.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
websocket/script/run.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🪛 yamllint (1.35.1)
websocket/output/conf.yaml
[error] 6-6: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci-websocket / ci
🔇 Additional comments (16)
websocket/docker-compose.yml (3)
9-9: Port mapping conflicts with existing benchmark code.The port mapping should be updated to 8080 to align with the existing benchmark code, as previously discussed.
- - "8000:8000" + - "8080:8080"
16-19: Enhance Redis configuration for production use.The Redis configuration needs improvements for production readiness as previously discussed.
21-25: Security and versioning concerns in fake-gcs configuration.The configuration has security and versioning concerns as previously discussed.
websocket/benches/readme.adoc (2)
1-8: LGTM! Well-structured document header.The document follows AsciiDoc best practices with proper heading hierarchy and table of contents placement.
26-82: Add performance baselines and complexity analysis.The benchmark descriptions would be more useful with performance expectations and resource requirements.
For each benchmark section, add:
- Expected baseline performance
- Time/space complexity
- Resource requirements
- Known limitations
ui/src/lib/yjs/useYjsSetup.ts (4)
16-16: LGTM! Clean authentication integration.The authentication hook integration is well-structured and follows React hooks best practices.
48-60: LGTM! Well-structured sync event handling.The sync event handler properly initializes the workflow when needed and manages the sync state.
75-75: LGTM! Proper dependency management.The addition of
getAccessTokento the effect dependencies is correct and follows React hooks best practices.
37-46: Verify WebSocket URL compatibility with yrs-warp.Since this PR is part of the WebSocket server refactoring to yrs-warp, let's verify that the WebSocket URL format and parameters are compatible with the new server implementation.
✅ Verification successful
WebSocket URL format and parameters are compatible with yrs-warp implementation ✓
The client's WebSocket URL format (
${projectId}:${workflowId}) and authentication parameters align perfectly with the server's route pattern (/:doc_id) and auth support. The server explicitly handles the:mainsuffix, ensuring seamless integration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check WebSocket configuration across the codebase echo "Checking WebSocket configuration..." # Look for WebSocket URL format in configuration files rg -A 5 'websocket.*=' --type yaml --type typescript --type javascript # Look for yrs-warp WebSocket handler implementation rg -A 10 'ws_handler|WebSocket' --type rustLength of output: 8974
websocket/script/run.sh (1)
1-1: Enhance script robustness and add proper shell directives.The script needs basic shell script best practices and error handling to ensure reliable execution.
Apply these improvements as previously suggested:
+#!/bin/bash +set -e + +# Verify binary exists and is executable +if [ ! -x "./main" ]; then + echo "Error: main binary not found or not executable" + exit 1 +fi + ./main🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
websocket/output/run.sh (1)
1-1: Add error handling and shell specificationThe run script needs improvements for better reliability.
+#!/bin/bash +set -e # Exit on error + +# Verify binary exists and is executable +if [ ! -x "./main" ]; then + echo "Error: main binary not found or not executable" + exit 1 +fi + ./main🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
websocket/output/conf.yaml (1)
4-6: 🛠️ Refactor suggestionImprove GCS configuration security and flexibility
The GCS configuration should support environment variables and include additional security parameters.
gcs: - bucket_name: yrs-dev - endpoint: http://localhost:4443 + bucket_name: ${REEARTH_FLOW_GCS_BUCKET_NAME:-yrs-dev} + endpoint: ${REEARTH_FLOW_GCS_ENDPOINT:-http://localhost:4443} + timeout_seconds: ${REEARTH_FLOW_GCS_TIMEOUT:-30} + retry_count: ${REEARTH_FLOW_GCS_RETRY:-3}Let's verify the GCS configuration usage:
🧰 Tools
🪛 yamllint (1.35.1)
[error] 6-6: no new line character at the end of file
(new-line-at-end-of-file)
websocket/build.sh (1)
1-6: Add error handling and shell specificationThe build script needs improvements in error handling and shell specification.
+#!/bin/bash +set -e # Exit on error + +# Clean previous build +rm -rf output + mkdir output -cp script/* output 2>/dev/null -cp -r src/conf.yaml output/ 2>/dev/null +# Copy scripts if directory exists +if [ -d "script" ]; then + cp script/* output +fi + +# Copy config if exists +if [ -f "src/conf.yaml" ]; then + cp -r src/conf.yaml output/ +fi + cargo build --bin main --release + +# Verify binary exists +if [ ! -f "target/release/main" ]; then + echo "Build failed: main binary not found" + exit 1 +fi + cp target/release/main output chmod +x output/*🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
websocket/Cargo.toml (3)
1-11:⚠️ Potential issueUpdate package metadata to match the project context
The current metadata appears to be copied from the
yrs-warpproject without proper adaptation.[package] name = "websocket" version = "0.1.0" edition = "2021" description = "Yrs synchronization protocol using Warp web sockets" license = "MIT" -authors = ["Bartosz Sypytkowski <[email protected]>"] -keywords = ["crdt", "yrs", "warp"] -homepage = "https://github.com/y-crdt/yrs-warp/" -repository = "https://github.com/y-crdt/yrs-warp/" +authors = ["Re:Earth Flow Contributors"] +keywords = ["crdt", "yrs", "warp", "reearth"] +homepage = "https://github.com/reearth/reearth-flow" +repository = "https://github.com/reearth/reearth-flow" readme = "./README.md"Likely invalid or redundant comment.
19-50: Review dependency version constraintsSeveral dependencies have very specific version constraints that might be too restrictive:
tokio 1.43could be1.0serde 1.0.217could be1.0reqwest 0.12.12is a future version (current latest is 0.11.x)Let's verify the latest versions:
40-43: Add security configurations for external servicesThe integration with Google Cloud Storage and Redis requires careful security configuration:
- Ensure proper authentication and authorization mechanisms
- Verify secure connection settings (TLS/SSL)
Let's verify the security implementations:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
websocket/src/conf.rs (3)
21-40: Add documentation and consider deriving additional traits.While the structs are well-organized, consider these improvements:
- Add documentation comments (
///) to describe each struct and its fields- Consider deriving additional useful traits like
CloneandPartialEq+/// Configuration for authentication #[derive(Debug, Deserialize)] +#[derive(Clone, PartialEq)] pub struct AuthConfig { + /// URL of the authentication service pub url: String, + /// Timeout in milliseconds for auth requests pub timeout_ms: u64, }
97-104: Consolidate logging logic.The logging of configuration values is duplicated between the .env and default paths. Consider extracting this into a separate method to maintain DRY principles.
+impl Config { + fn log_config(&self) { + info!("redis: {:?}", self.redis); + info!("gcs: {:?}", self.gcs); + info!("app: {:?}", self.app); + #[cfg(feature = "auth")] + info!("auth: {:?}", self.auth); + } +}
109-149: Reduce code duplication in Default implementation.The current implementation duplicates configuration code between feature-gated blocks. Consider using a builder pattern or helper methods to reduce duplication.
impl Default for Config { fn default() -> Self { - #[cfg(feature = "auth")] - let config = Config { + let mut config = Config { redis: RedisConfig { url: DEFAULT_REDIS_URL.to_string(), ttl: DEFAULT_REDIS_TTL, }, gcs: GcsConfig { bucket_name: DEFAULT_GCS_BUCKET.to_string(), endpoint: Some(DEFAULT_GCS_ENDPOINT.to_string()), }, - auth: AuthConfig { - url: DEFAULT_AUTH_URL.to_string(), - timeout_ms: DEFAULT_AUTH_TIMEOUT_MS, - }, app: AppConfig { env: DEFAULT_APP_ENV.to_string(), origins: DEFAULT_ORIGINS.to_string(), }, }; - #[cfg(not(feature = "auth"))] - let config = Config { - redis: RedisConfig { - url: DEFAULT_REDIS_URL.to_string(), - ttl: DEFAULT_REDIS_TTL, - }, - gcs: GcsConfig { - bucket_name: DEFAULT_GCS_BUCKET.to_string(), - endpoint: Some(DEFAULT_GCS_ENDPOINT.to_string()), - }, - app: AppConfig { - env: DEFAULT_APP_ENV.to_string(), - origins: DEFAULT_ORIGINS.to_string(), - }, - }; + #[cfg(feature = "auth")] + { + config.auth = AuthConfig { + url: DEFAULT_AUTH_URL.to_string(), + timeout_ms: DEFAULT_AUTH_TIMEOUT_MS, + }; + } config } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/internal/app/app.go(2 hunks)websocket/.env-sample(1 hunks)websocket/src/conf.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- api/internal/app/app.go
- websocket/.env-sample
🔇 Additional comments (2)
websocket/src/conf.rs (2)
1-20: LGTM! Well-structured imports and constants.The imports are appropriate, and the constants are well-organized with reasonable default values. Good use of feature flags for auth-related constants.
51-60: Avoid logging sensitive configuration values.The current logging implementation might expose sensitive information in logs.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
websocket/src/auth.rs (1)
29-47: Enhance error context and logging.The implementation is solid, but consider these improvements:
- Add more context to errors using
.context()or.with_context()- Include token length (not the actual token) in debug logging for better debugging
pub async fn verify_token(&self, token: &str) -> Result<bool> { { let response = self .client .post(&self.config.url) .header("Authorization", format!("Bearer {}", token)) .send() .await - ?; + .with_context(|| format!("Failed to send auth request to {}", self.config.url))?; - tracing::debug!("response: {:?}", response); + tracing::debug!( + "Auth response: status={}, token_len={}", + response.status(), + token.len() + ); if !response.status().is_success() { return Ok(false); } let auth_response = response.json::<AuthResponse>().await?; Ok(auth_response.authorized) } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/internal/app/app.go(2 hunks)websocket/src/auth.rs(1 hunks)
🔇 Additional comments (2)
websocket/src/auth.rs (2)
1-15: LGTM! Clean and well-organized struct definitions.The imports are appropriate and the structs are well-defined with proper derive macros.
18-27: Avoid usingexpectfor error handling.Using
expecthere will cause a panic if the client cannot be created. It's better to handle the potential error gracefully.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
api/internal/app/app.go (1)
103-105:⚠️ Potential issueCritical: Implement proper token verification.
The endpoint always returns
authorized: truewithout any token verification. This is a significant security risk as it bypasses authentication completely.-apiPrivate.POST("/verify/ws-token", func(c echo.Context) error { - return c.JSON(http.StatusOK, echo.Map{"authorized": true}) -}) +apiPrivate.POST("/verify/ws-token", func(c echo.Context) error { + authInfo := adapter.GetAuthInfo(c.Request().Context()) + if authInfo == nil { + return c.JSON(http.StatusUnauthorized, echo.Map{"authorized": false}) + } + return c.JSON(http.StatusOK, echo.Map{"authorized": true}) +})websocket/src/bin/main.rs (2)
36-37: 🛠️ Refactor suggestionMove hardcoded values to configuration.
Constants like
BUCKET_NAMEandPORTshould be configurable for better flexibility and maintainability.-const BUCKET_NAME: &str = "yrs-dev"; -const PORT: &str = "8000"; +use crate::conf::Config;Then use these values from the config:
let bucket_name = &config.gcs.bucket_name; let port = &config.app.port;
175-178: 🛠️ Refactor suggestionImprove error handling for server startup.
Using
unwrap()on server startup operations could cause the application to panic. Implement proper error handling.- let listener = tokio::net::TcpListener::bind(format!("0.0.0.0:{}", PORT)) - .await - .unwrap(); - axum::serve(listener, app).await.unwrap(); + let listener = match tokio::net::TcpListener::bind(format!("0.0.0.0:{}", PORT)).await { + Ok(l) => l, + Err(e) => { + tracing::error!("Failed to bind to address: {}", e); + return; + } + }; + if let Err(e) = axum::serve(listener, app).await { + tracing::error!("Server error: {}", e); + }websocket/src/conf.rs (1)
51-60:⚠️ Potential issueMask sensitive configuration values in logs.
The current logging implementation might expose sensitive information. Consider masking sensitive values or logging at a debug level only.
info!( - redis_url = ?env::var("REEARTH_FLOW_REDIS_URL").ok(), + redis_url = ?env::var("REEARTH_FLOW_REDIS_URL").ok().map(|_| "***"), redis_ttl = ?env::var("REEARTH_FLOW_REDIS_TTL").ok(), gcs_bucket = ?env::var("REEARTH_FLOW_GCS_BUCKET_NAME").ok(), - gcs_endpoint = ?env::var("REEARTH_FLOW_GCS_ENDPOINT").ok(), - auth_url = ?env::var("REEARTH_FLOW_AUTH_URL").ok(), + gcs_endpoint = ?env::var("REEARTH_FLOW_GCS_ENDPOINT").ok().map(|_| "***"), + auth_url = ?env::var("REEARTH_FLOW_AUTH_URL").ok().map(|_| "***"), auth_timeout = ?env::var("REEARTH_FLOW_AUTH_TIMEOUT_MS").ok(), app_env = ?env::var("REEARTH_FLOW_APP_ENV").ok(), - origins = ?env::var("REEARTH_FLOW_ORIGINS").ok(), + origins = ?env::var("REEARTH_FLOW_ORIGINS").ok().map(|_| "***"), );
🧹 Nitpick comments (14)
websocket/src/conn.rs (2)
1-1: Consider removing the#![allow(dead_code)]attribute if unneeded.
If this file is expected to be used in production, we usually avoid suppressing warnings such asdead_code. Over time, they can hide potentially obsolete or unused items.
61-77: Poll method can silently discard errors ifsub.completed()fails early.
Currently, thepollmethod reassignscompletion_futurefromsub.completed()and then returnsPoll::Ready(Ok(()))if no future is present. Consider how partial initialization or early disconnection might propagate error information.websocket/src/broadcast/pool.rs (2)
70-83: Validate broadcast configuration for multi-region or multi-instance setups.
When creating a newBroadcastGroup, theredis_configis set (lines 77–79). In multi-instance deployments of WebSocket servers, ensure that all servers share the same config and redis endpoints to avoid data partition issues.
90-91: Retaining only non-empty groups can result in short-lived ephemeral groups.
If your use case involves frequently toggling between zero and non-zero connection counts, you may see repeated group creation and cleanup. Consider whether a small grace period before cleanup might avoid excessive churn.websocket/src/ws.rs (2)
16-22: Consider returning aResult<WarpConn, Error>fromnew.
TheWarpConn::newmethod (lines 17–21) creates a connection unconditionally. If something fails during the subscription process, returning aResultmight give better error visibility.
98-117: Investigate handling of non-binary messages inWarpStream.
Currently,Ping,Pong,Text, andClosecause the function to yieldPoll::Pendingand re-wake the waker without further action (lines 108–111). This can create repeatedPoll::Pendingstates if the remote continuously sends text or control frames. Consider more explicit handling to avoid potential busy loops.websocket/src/broadcast/group.rs (5)
1-22: Consider removing unused imports.
Line 2 is commented out, which may indicate dead code. If this import is no longer needed, removing it helps keep the code clean.
23-41: Document the meaning of crucial fields inRedisConfigandBroadcastConfig.
While the structs are self-explanatory for seasoned developers, it could be beneficial to add doc comments clarifying usage details (e.g., seconds forttl, necessity ofdoc_namewhen persistent storage is enabled). This helps future maintainers understand the rationale and usage scope of these fields.
60-159: AssessOrdering::Relaxedfor connection counters.
The use ofOrdering::Relaxedallows for unordered atomic operations, which is typically acceptable for simple counters but can obscure more complex concurrency needs. If the code requires any memory-order guarantees (e.g., ensuring a consistent view of memory), consider a stronger ordering.
311-421: Clarify error-handling and termination insubscribe_withandhandle_msg.
If downstream sink sending orMessage::decode_v1fails, the code logs or returns an error and continues. Consider whether you want to terminate the connection, attempt to recover, or notify peers. A consistent error-handling policy ensures predictable system behavior.
423-449: Validate cleanup logic inDrop.
Manually aborting tasks indropcan leave partially processed messages or subscription states. If further cleanup is needed (e.g., flushing buffers), consider using structured shutdown signals before aborting tasks to ensure a graceful exit.websocket/src/auth/mod.rs (2)
17-27: Consider handling failure more gracefully in thenewconstructor.
Theexpect("Failed to create HTTP client")call will panic on error. In production systems, you may prefer returning aResult<Self>or logging the error and continuing without authentication if feasible.
29-48: Improve error visibility inverify_token.
Currently, any non-2xx HTTP status leads to a simpleOk(false)outcome. You might log or surface the reason for the failure to help diagnose connectivity or token issues. Adding structured retries or exponential backoff might also be beneficial for transient network problems.websocket/src/lib.rs (1)
4-16: Document the new modules and feature flags for clarity.
Adding doc comments or a summary block explaining the purpose of each module (e.g.,auth,broadcast, etc.) will help future maintainers quickly locate relevant functionalities (especially important for conditional modules under feature flags).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
api/internal/app/app.go(2 hunks)websocket/src/auth/mod.rs(1 hunks)websocket/src/bin/main.rs(1 hunks)websocket/src/broadcast/group.rs(1 hunks)websocket/src/broadcast/mod.rs(1 hunks)websocket/src/broadcast/pool.rs(1 hunks)websocket/src/conf.rs(1 hunks)websocket/src/conn.rs(1 hunks)websocket/src/lib.rs(1 hunks)websocket/src/ws.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- websocket/src/broadcast/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci-api / ci-api-test
🔇 Additional comments (11)
websocket/src/conn.rs (2)
30-45: Validate subscription initialization errors.
When subscribing to theBroadcastGroup(line 37, line 50), there is no handling for potential errors or misconfiguration. If subscription creation fails internally, it may result in runtime errors or undefined behavior downstream.Would you like a script to search for usage or error handling in
subscribecalls from other parts of the codebase to ensure they are always successful?
79-85: Ensure subscription cleanup is concurrency-safe.
Dropping theSubscriptiononDropis good practice. However, if multiple tasks hold references to thisConnection, ensure no concurrency conflicts or ownership contention can arise once the subscription is dropped.websocket/src/broadcast/pool.rs (2)
50-65: Double-check loaded document consistency.
This code loads the document from Google Cloud Storage (lines 53–65) but only logs a warning on failure. Consider whether the failing scenario should be escalated or retried, especially if synchronization or bridging logic depends on a successful load.
93-100: Potential race condition on simultaneous connection removals.
DashMapis concurrency-safe for updates, but if multiple tasks callremove_connectionat the same time, different concurrency paths may lead to unexpected cleanup or partial state changes if the doc ID key is removed. Recheckdecrement_connectionslogic for atomic updates.websocket/src/ws.rs (1)
47-81: Handle partial writes and large payloads inWarpSink.
Your code handles normal readiness and writes, but for large payloads or slow connections, partial writes can occur. Ensure thatpoll_readyandstart_sendhandle chunking if your environment allows extremely large binary messages.websocket/src/broadcast/group.rs (2)
43-55: Review concurrency safety ofunsafe impl Sendandunsafe impl Sync.
DeclaringBroadcastGroupexplicitly asSendandSyncwithunsafe implcan be risky if any non-Sync or non-Send fields are introduced later. Verifying all fields are safe for concurrent access is crucial to avoid undefined behavior.
160-309: Handle partial storage failures more robustly.
When Redis or GCS operations fail, the current fallback is to attempt loading from storage or simply log an error. This approach might lead to inconsistent states under partial successes. Consider introducing a retry or a more robust error-handling mechanism to ensure data consistency.websocket/src/auth/mod.rs (1)
1-15: EnsureAuthResponseis aligned with the authentication server contract.
When working with external authentication servers, verify theAuthResponsestruct matches the server’s response schema to avoid deserialization mismatches.websocket/src/lib.rs (1)
1-3: Imports look consistent.
ImportingArcandRwLockis sensible for concurrency. No issues spotted here.api/internal/app/app.go (1)
58-62:⚠️ Potential issueRestore proper JWT configuration.
The JWT configuration is commented out without a proper replacement. This could lead to security issues in production environments.
-// authConfig := []appx.JWTProvider{{ -// ISS: "https://reearth-oss-test.eu.auth0.com/", -// AUD: []string{"https://api.test.reearth.dev"}, -// JWKSURI: lo.ToPtr("https://reearth-oss-test.eu.auth0.com/.well-known/jwks.json"), -// }} +authConfig := cfg.Config.JWTProviders() +if len(authConfig) == 0 && cfg.Debug { + authConfig = []appx.JWTProvider{{ + ISS: os.Getenv("JWT_ISSUER"), + AUD: strings.Split(os.Getenv("JWT_AUDIENCE"), ","), + JWKSURI: lo.ToPtr(os.Getenv("JWT_JWKS_URI")), + }} +}Likely invalid or redundant comment.
websocket/src/conf.rs (1)
67-71: Improve error handling for numeric parsing.The current implementation silently ignores parsing errors. Consider logging these errors to help with debugging configuration issues.
if let Ok(ttl_str) = env::var("REEARTH_FLOW_REDIS_TTL") { - if let Ok(ttl) = ttl_str.parse() { + match ttl_str.parse() { + Ok(ttl) => { config.redis.ttl = ttl; + } + Err(e) => { + warn!("Failed to parse REEARTH_FLOW_REDIS_TTL: {}", e); + } } }
kasugamirai
force-pushed
the
feat/websocket-yrs
branch
2 times, most recently
from
8eb5735 to
0cee766
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
ui/src/lib/yjs/useYjsSetup.ts (1)
Line range hint
47-62: Add WebSocket connection error handling and fix indentation.The WebSocket connection should handle potential errors and the sync callback needs proper indentation.
); + yWebSocketProvider.on('connection-error', (error: Error) => { + console.error('WebSocket connection error:', error); + setIsSynced(false); + }); + + yWebSocketProvider.on('connection-close', () => { + setIsSynced(false); + }); + yWebSocketProvider.once("sync", () => { if (yWorkflows.length === 0) { yDoc.transact(() => { const yWorkflow = yWorkflowConstructor( DEFAULT_ENTRY_GRAPH_ID, "Main Workflow", ); yWorkflows.push([yWorkflow]); }); } setIsSynced(true); });🧰 Tools
🪛 GitHub Actions: ci
[warning] Code style issues found. Run Prettier with --write to fix formatting issues.
♻️ Duplicate comments (1)
websocket/output/conf.yaml (1)
1-3: 🛠️ Refactor suggestionEnhance Redis configuration security and flexibility
The Redis configuration should support environment variables for better security and deployment flexibility.
Apply this expanded diff to support environment variables for both URL and TTL:
redis: - url: redis://127.0.0.1:6379 - ttl: 3600 + url: ${REEARTH_FLOW_REDIS_URL:-redis://127.0.0.1:6379} + ttl: ${REEARTH_FLOW_REDIS_TTL:-3600}
🧹 Nitpick comments (10)
websocket/output/conf.yaml (1)
6-6: Add newline at end of fileAdd a newline character at the end of the file to comply with YAML best practices.
endpoint: http://localhost:4443 +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 6-6: no new line character at the end of file
(new-line-at-end-of-file)
ui/src/lib/yjs/useYjsSetup.ts (2)
6-6: Fix import order according to conventions.Move the
@flow/lib/authimport after@flow/global-constantsto maintain consistent import ordering.-import { useAuth } from "@flow/lib/auth"; import { config } from "@flow/config"; import { DEFAULT_ENTRY_GRAPH_ID } from "@flow/global-constants"; +import { useAuth } from "@flow/lib/auth";🧰 Tools
🪛 GitHub Check: ci-ui / ci
[warning] 6-6:
@flow/lib/authimport should occur after import of@flow/global-constants🪛 GitHub Actions: ci
[warning] Code style issues found. Run Prettier with --write to fix formatting issues.
73-76: Enhance cleanup to handle all resources.The cleanup function should handle all resources, including resetting the sync state.
return () => { - yWebSocketProvider?.destroy(); // Cleanup on unmount + if (yWebSocketProvider) { + yWebSocketProvider.destroy(); + setIsSynced(false); + } }; }, [projectId, workflowId, getAccessToken]);🧰 Tools
🪛 GitHub Actions: ci
[warning] Code style issues found. Run Prettier with --write to fix formatting issues.
websocket/script/run.sh (1)
1-1: Add build documentation.Add a comment section to document the build requirements and process.
#!/bin/bash set -euo pipefail + +# This script runs the WebSocket server for real-time collaboration. +# Requirements: +# - Rust toolchain installed +# - Project built using 'cargo build' +# +# For development: +# 1. cd websocket +# 2. cargo build +# 3. ./script/run.sh + # Get the directory where the script is located🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
websocket/docker-compose.yml (1)
29-30: Enhance volume configuration for production use.Consider adding volume configuration options for better data management:
- Specify a volume driver for managed storage
- Add backup-friendly options
volumes: gcs-data: + driver: local + driver_opts: + type: none + device: ${PWD}/data/gcs + o: bindwebsocket/benches/server_bench.rs (2)
44-50: Add graceful error handling for connection failures.
If the server is unavailable or crashes,connect_async(url).await.expect("Failed to connect")will panic. Consider logging an error and allowing the benchmark logic to continue or retry.async fn connect_client( doc_id: &str, ) -> tokio_tungstenite::WebSocketStream<tokio_tungstenite::MaybeTlsStream<tokio::net::TcpStream>> { let url = format!("ws://127.0.0.1:8080/{}", doc_id); - let (ws_stream, _) = connect_async(url).await.expect("Failed to connect"); + let (ws_stream, _) = match connect_async(url).await { + Ok((ws, _)) => ws, + Err(e) => { + tracing::error!("Connection failed: {}", e); + // Possibly bail out of the benchmark or retry + panic!("Unable to connect for benchmark"); + } + }; ws_stream }
316-354: Clarify memory usage for large document updates.
When constructing large updates within a loop, memory usage grows quickly. Consider controlling memory allocations or chunking large updates to avoid big spikes that can distort benchmarking results.websocket/build.sh (1)
1-6: Add shebang and starter error handling.
Without a shebang (#!/bin/bashor similar), ShellCheck flags SC2148. Also, addingset -ewould immediately fail on errors rather than silently continuing.+#!/bin/bash +set -e mkdir output cp script/* output 2>/dev/null cp -r src/conf.yaml output/ 2>/dev/null cargo build --bin main --release cp target/release/main output chmod +x output/*🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
websocket/Cargo.toml (1)
19-51: Review dependency bloat & evaluate optional features.
Currently, dependencies includeredis,google-cloud-storage, andsmallvecwith multiple features. Some might be toggled via optional features for minimal builds. This helps limit build times and reduce the attack surface for production usage.websocket/benches/readme.adoc (1)
9-25: Add prerequisites and detailed result interpretation.Consider adding:
- System requirements (CPU, memory, network)
- Required dependencies and versions
- Example benchmark results
- Troubleshooting common issues
== Overview The benchmark suite tests various aspects of the WebSocket server performance, including connection handling, document synchronization, and concurrent operations. +== Prerequisites + +* Rust toolchain (minimum version: X.Y.Z) +* Minimum system requirements: +** CPU: 2+ cores recommended +** Memory: 4GB+ recommended +** Network: Stable connection required + == Running Benchmarks
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
websocket/.DS_Storeis excluded by!**/.DS_Storewebsocket/Cargo.lockis excluded by!**/*.lock,!**/*.lockwebsocket/README.mdis excluded by!**/*.md
📒 Files selected for processing (82)
api/internal/app/app.go(1 hunks)ui/src/lib/yjs/useYjsSetup.ts(5 hunks)websocket/.dockerignore(0 hunks)websocket/.env-sample(1 hunks)websocket/.gitignore(1 hunks)websocket/.taplo.toml(0 hunks)websocket/Cargo.toml(1 hunks)websocket/Dockerfile(1 hunks)websocket/LICENSE(1 hunks)websocket/Makefile.toml(0 hunks)websocket/README.adoc(1 hunks)websocket/Redis.Dockerfile(0 hunks)websocket/Websocket.Dockerfile(0 hunks)websocket/app/Cargo.toml(0 hunks)websocket/app/examples/readme.adoc(0 hunks)websocket/app/examples/room_client.rs(0 hunks)websocket/app/examples/session_client.rs(0 hunks)websocket/app/readme.adoc(0 hunks)websocket/app/src/bin/main.rs(0 hunks)websocket/app/src/config.rs(0 hunks)websocket/app/src/errors.rs(0 hunks)websocket/app/src/handlers/cleanup.rs(0 hunks)websocket/app/src/handlers/heartbeat.rs(0 hunks)websocket/app/src/handlers/message_handler.rs(0 hunks)websocket/app/src/handlers/mod.rs(0 hunks)websocket/app/src/handlers/room_handler.rs(0 hunks)websocket/app/src/handlers/socket_handler.rs(0 hunks)websocket/app/src/handlers/types.rs(0 hunks)websocket/app/src/lib.rs(0 hunks)websocket/app/src/middleware/mod.rs(0 hunks)websocket/app/src/middleware/trace.rs(0 hunks)websocket/app/src/room.rs(0 hunks)websocket/app/src/routes.rs(0 hunks)websocket/app/src/state.rs(0 hunks)websocket/benches/readme.adoc(1 hunks)websocket/benches/server_bench.rs(1 hunks)websocket/build.sh(1 hunks)websocket/clippy.toml(0 hunks)websocket/conf/conf.yaml(0 hunks)websocket/config/config.toml(0 hunks)websocket/crates/infra/Cargo.toml(0 hunks)websocket/crates/infra/src/lib.rs(0 hunks)websocket/crates/infra/src/persistence/editing_session.rs(0 hunks)websocket/crates/infra/src/persistence/event_handler.rs(0 hunks)websocket/crates/infra/src/persistence/gcs/gcs_client.rs(0 hunks)websocket/crates/infra/src/persistence/gcs/mod.rs(0 hunks)websocket/crates/infra/src/persistence/local_storage/local_client.rs(0 hunks)websocket/crates/infra/src/persistence/local_storage/mod.rs(0 hunks)websocket/crates/infra/src/persistence/mod.rs(0 hunks)websocket/crates/infra/src/persistence/project_repository.rs(0 hunks)websocket/crates/infra/src/persistence/redis/connection.rs(0 hunks)websocket/crates/infra/src/persistence/redis/default_key_manager.rs(0 hunks)websocket/crates/infra/src/persistence/redis/errors.rs(0 hunks)websocket/crates/infra/src/persistence/redis/flow_project_lock.rs(0 hunks)websocket/crates/infra/src/persistence/redis/flow_project_redis_data_manager.rs(0 hunks)websocket/crates/infra/src/persistence/redis/keys.rs(0 hunks)websocket/crates/infra/src/persistence/redis/mod.rs(0 hunks)websocket/crates/infra/src/persistence/redis/types.rs(0 hunks)websocket/crates/infra/src/persistence/redis/updates.rs(0 hunks)websocket/crates/infra/src/persistence/repository.rs(0 hunks)websocket/crates/infra/src/types/mod.rs(0 hunks)websocket/crates/infra/src/types/project.rs(0 hunks)websocket/crates/infra/src/types/snapshot.rs(0 hunks)websocket/crates/infra/src/types/user.rs(0 hunks)websocket/crates/infra/src/types/workspace.rs(0 hunks)websocket/crates/infra/src/utils.rs(0 hunks)websocket/crates/services/Cargo.toml(0 hunks)websocket/crates/services/examples/edit_session_service.rs(0 hunks)websocket/crates/services/examples/project_service.rs(0 hunks)websocket/crates/services/examples/readme.adoc(0 hunks)websocket/crates/services/src/error.rs(0 hunks)websocket/crates/services/src/lib.rs(0 hunks)websocket/crates/services/src/manage_project_edit_session.rs(0 hunks)websocket/crates/services/src/project.rs(0 hunks)websocket/crates/services/src/types.rs(0 hunks)websocket/docker-compose.yml(1 hunks)websocket/output/conf.yaml(1 hunks)websocket/output/run.sh(1 hunks)websocket/redis.conf(0 hunks)websocket/rustfmt.toml(0 hunks)websocket/script/run.sh(1 hunks)websocket/src/auth/mod.rs(1 hunks)
⛔ Files not processed due to max files limit (14)
- websocket/src/bin/main.rs
- websocket/src/broadcast/group.rs
- websocket/src/broadcast/mod.rs
- websocket/src/broadcast/pool.rs
- websocket/src/conf.rs
- websocket/src/conn.rs
- websocket/src/lib.rs
- websocket/src/storage/gcs/mod.rs
- websocket/src/storage/kv/error.rs
- websocket/src/storage/kv/keys.rs
- websocket/src/storage/kv/mod.rs
- websocket/src/storage/mod.rs
- websocket/src/storage/sqlite/mod.rs
- websocket/src/ws.rs
💤 Files with no reviewable changes (66)
- websocket/Redis.Dockerfile
- websocket/app/examples/readme.adoc
- websocket/app/src/middleware/trace.rs
- websocket/rustfmt.toml
- websocket/crates/services/examples/readme.adoc
- websocket/app/Cargo.toml
- websocket/crates/infra/Cargo.toml
- websocket/crates/infra/src/persistence/event_handler.rs
- websocket/app/examples/room_client.rs
- websocket/crates/infra/src/utils.rs
- websocket/.taplo.toml
- websocket/app/src/bin/main.rs
- websocket/app/readme.adoc
- websocket/.dockerignore
- websocket/crates/infra/src/lib.rs
- websocket/redis.conf
- websocket/crates/infra/src/persistence/gcs/mod.rs
- websocket/config/config.toml
- websocket/conf/conf.yaml
- websocket/app/src/routes.rs
- websocket/crates/infra/src/persistence/redis/connection.rs
- websocket/crates/services/examples/edit_session_service.rs
- websocket/clippy.toml
- websocket/crates/services/src/error.rs
- websocket/app/src/middleware/mod.rs
- websocket/app/src/handlers/mod.rs
- websocket/crates/infra/src/types/mod.rs
- websocket/crates/infra/src/types/project.rs
- websocket/crates/services/examples/project_service.rs
- websocket/crates/infra/src/persistence/local_storage/mod.rs
- websocket/crates/infra/src/persistence/editing_session.rs
- websocket/app/src/errors.rs
- websocket/app/src/handlers/room_handler.rs
- websocket/app/src/handlers/cleanup.rs
- websocket/crates/infra/src/persistence/project_repository.rs
- websocket/Websocket.Dockerfile
- websocket/crates/services/src/lib.rs
- websocket/app/src/handlers/types.rs
- websocket/app/src/config.rs
- websocket/crates/services/src/types.rs
- websocket/crates/infra/src/persistence/redis/default_key_manager.rs
- websocket/crates/infra/src/persistence/redis/errors.rs
- websocket/app/src/handlers/socket_handler.rs
- websocket/crates/infra/src/persistence/repository.rs
- websocket/crates/infra/src/persistence/redis/keys.rs
- websocket/crates/infra/src/persistence/redis/types.rs
- websocket/app/src/handlers/heartbeat.rs
- websocket/Makefile.toml
- websocket/crates/infra/src/persistence/redis/mod.rs
- websocket/crates/infra/src/types/user.rs
- websocket/crates/infra/src/persistence/mod.rs
- websocket/app/examples/session_client.rs
- websocket/crates/infra/src/types/snapshot.rs
- websocket/app/src/lib.rs
- websocket/app/src/handlers/message_handler.rs
- websocket/crates/infra/src/persistence/redis/flow_project_lock.rs
- websocket/crates/infra/src/persistence/redis/flow_project_redis_data_manager.rs
- websocket/app/src/room.rs
- websocket/app/src/state.rs
- websocket/crates/infra/src/persistence/gcs/gcs_client.rs
- websocket/crates/infra/src/persistence/local_storage/local_client.rs
- websocket/crates/infra/src/persistence/redis/updates.rs
- websocket/crates/services/Cargo.toml
- websocket/crates/services/src/manage_project_edit_session.rs
- websocket/crates/services/src/project.rs
- websocket/crates/infra/src/types/workspace.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- websocket/.gitignore
- websocket/LICENSE
- api/internal/app/app.go
- websocket/README.adoc
- websocket/Dockerfile
- websocket/src/auth/mod.rs
- websocket/.env-sample
🧰 Additional context used
🪛 GitHub Check: ci-ui / ci
ui/src/lib/yjs/useYjsSetup.ts
[warning] 6-6:
@flow/lib/auth import should occur after import of @flow/global-constants
🪛 GitHub Actions: ci
ui/src/lib/yjs/useYjsSetup.ts
[warning] Code style issues found. Run Prettier with --write to fix formatting issues.
🪛 Shellcheck (0.10.0)
websocket/build.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
websocket/output/run.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
websocket/script/run.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🪛 YAMLlint (1.35.1)
websocket/output/conf.yaml
[error] 6-6: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (13)
websocket/script/run.sh (1)
1-1: Verify binary build configuration.Ensure that the binary target is properly configured in
Cargo.tomland the build process is working correctly.Run the following script to verify the binary configuration:
#!/bin/bash # Description: Verify binary configuration and build process # Check Cargo.toml configuration echo "Checking Cargo.toml configuration..." if [ -f "websocket/Cargo.toml" ]; then rg -A 5 '^\[\[bin\]\]|^name\s*=\s*"main"' websocket/Cargo.toml fi # Check if binary exists echo "Checking binary existence..." if [ -f "websocket/target/debug/main" ]; then file "websocket/target/debug/main" fi # Check build script echo "Checking build script..." if [ -f "websocket/build.sh" ]; then cat "websocket/build.sh" fi🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
websocket/output/run.sh (1)
1-1: 🛠️ Refactor suggestionEnhance script robustness and add environment validation.
The current script needs significant improvements for reliability and proper environment setup:
Apply this diff to enhance the script:
-./main +#!/bin/bash +set -euo pipefail + +# Configuration +REQUIRED_ENV_VARS=( + "GCS_BUCKET" # For document storage + "AUTH_ENABLED" # For authentication settings + "WS_PORT" # For WebSocket server port +) + +# Function to check if environment variables are set +check_env_vars() { + local missing_vars=() + for var in "${REQUIRED_ENV_VARS[@]}"; do + if [[ -z "${!var:-}" ]]; then + missing_vars+=("$var") + fi + done + + if [[ ${#missing_vars[@]} -gt 0 ]]; then + echo "Error: Required environment variables not set:" + printf '%s\n' "${missing_vars[@]}" + exit 1 + fi +} + +# Function to verify binary +check_binary() { + if [[ ! -f "./main" ]]; then + echo "Error: main binary not found" + echo "Please build the project first" + exit 1 + fi + + if [[ ! -x "./main" ]]; then + echo "Error: main binary not executable" + echo "Run: chmod +x ./main" + exit 1 + fi +} + +# Main +main() { + echo "Starting WebSocket server..." + echo "Checking environment..." + check_env_vars + + echo "Verifying binary..." + check_binary + + echo "Running server on port $WS_PORT..." + ./main +} + +main "$@"This enhancement:
- Adds proper shell options for error handling
- Validates required environment variables
- Checks binary existence and permissions
- Provides helpful error messages and instructions
- Improves logging for better observability
Let's verify the environment variables used in the WebSocket server:
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
websocket/docker-compose.yml (5)
9-9: Update WebSocket port to 8080 for consistency.The port should be changed to 8080 to align with existing benchmark code and documentation.
- - "8000:8000" + - "8080:8080"
16-19: Enhance Redis configuration for production readiness.The Redis configuration needs improvements for security and reliability:
- Add volume persistence
- Configure password protection
- Set resource limits
21-25: Security and versioning concerns in fake-gcs configuration.
- Using HTTP on port 4443 (typically HTTPS) might cause confusion
- Missing version tag for the image could lead to unexpected changes
26-27: Validate GCS data volume mount permissions.Ensure the volume mount has appropriate permissions and ownership for the GCS server process.
Let's check the Dockerfile for user/group configuration:
10-11: Validate required environment variables.Add environment variable validation to ensure all required variables are present at startup.
Let's check the required environment variables:
websocket/benches/server_bench.rs (2)
193-235: Add resource handling for high concurrency.
Multiple concurrent clients spawn tasks, each establishing a connection, sending updates, and awaiting a response. Consider pooling connections or adding graceful shutdown logic to avoid potential resource leaks or long-running connections if an error occurs.
8-13: Consider verifying shorter timeout settings with real-world usage.
Although rapid feedback is ideal for benchmark scenarios, the reduced timeouts may make benchmarks prone to timing flakiness or false negatives in constrained environments.Please run a few repeated tests on diverse hardware/CI runners to validate that these short timeouts, especially
TIMEOUT_DURATIONof 50ms, don’t result in inconsistent benchmark failures.✅ Verification successful
Short timeouts are appropriate for benchmark scenarios
The timeout values are specifically optimized for benchmarking and follow a proper hierarchy (50ms > 20ms > 10ms) to prevent race conditions. These values are used only in benchmark code and are comparable to other timing patterns in the codebase's test scenarios.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Test reliability of short timeouts with repeated runs for i in {1..5}; do cargo bench --bench server_bench || { echo "Run $i failed"; } doneLength of output: 919
Script:
#!/bin/bash # Search for usage of timeout constants and related timing patterns echo "=== Searching for timeout constant usage ===" rg "TIMEOUT_DURATION|CONNECTION_DELAY|CLOSE_DELAY" -A 2 -B 2 echo -e "\n=== Searching for timing-related documentation ===" rg -l "timeout|delay" README.md docs/ 2>/dev/null echo -e "\n=== Looking for other timeout patterns ===" rg "Duration::from_" -A 1 -B 1Length of output: 8246
websocket/Cargo.toml (2)
60-62: Benchmark harness approach is good to minimize overhead.
Using[[bench]]withharness = falsedirectly integrates Criterion’s test runner, reducing extra overhead. This is a neat approach for micro-benchmarks.
1-10: 🛠️ Refactor suggestionUpdate metadata for project consistency.
The metadata (homepage, repository, authors) still referencesyrs-warp. Since this code is part of the Re:Earth Flow project, align it with the actual repository and contributors.[package] name = "websocket" version = "0.1.0" edition = "2021" description = "Yrs synchronization protocol using Warp web sockets" license = "MIT" -authors = ["Bartosz Sypytkowski <[email protected]>"] -keywords = ["crdt", "yrs", "warp"] -homepage = "https://github.com/y-crdt/yrs-warp/" -repository = "https://github.com/y-crdt/yrs-warp/" +authors = ["Re:Earth Flow Contributors"] +keywords = ["crdt", "yrs", "warp", "reearth"] +homepage = "https://github.com/reearth/reearth-flow" +repository = "https://github.com/reearth/reearth-flow" readme = "./README.md"Likely invalid or redundant comment.
websocket/benches/readme.adoc (2)
1-8: Well-structured document format!The document follows AsciiDoc best practices with proper headers and table of contents setup.
26-82: Add performance baselines and complexity analysis.The benchmark descriptions would be more useful with performance expectations and resource requirements.
For each benchmark section, add:
- Expected baseline performance
- Time/space complexity
- Resource requirements
- Known limitations
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
ui/src/lib/yjs/useYjsSetup.ts (1)
17-17:⚠️ Potential issueAdd error handling for token retrieval.
The token retrieval should include error handling to gracefully handle authentication failures.
api/internal/app/app.go (2)
58-62:⚠️ Potential issueReplace hardcoded JWT configuration with configurable values.
The current implementation hardcodes test environment JWT configuration values. This reduces flexibility and could lead to security issues if deployed to other environments.
103-105:⚠️ Potential issueImplement proper token verification for WebSocket authentication.
The current endpoint always returns
authorized: truewithout any actual token verification. This is a significant security risk as it bypasses authentication completely.
🧹 Nitpick comments (2)
ui/src/lib/yjs/useYjsSetup.ts (2)
6-6: Reorder imports according to conventions.Move the
@flowimports after the external package imports to maintain consistent import ordering.import { useParams } from "@tanstack/react-router"; import { useEffect, useState } from "react"; import { WebsocketProvider } from "y-websocket"; import * as Y from "yjs"; -import { useAuth } from "@flow/lib/auth"; import { config } from "@flow/config"; import { DEFAULT_ENTRY_GRAPH_ID } from "@flow/global-constants"; +import { useAuth } from "@flow/lib/auth"; import { yWorkflowConstructor } from "./conversions";🧰 Tools
🪛 GitHub Check: ci-ui / ci
[warning] 6-6:
@flow/lib/authimport should occur after import of@flow/global-constants
76-76: Consider memoizing getAccessToken to prevent unnecessary reconnections.Adding
getAccessTokento the dependency array is correct, but it could cause unnecessary WebSocket reconnections if the function reference changes frequently. Consider memoizing the function in the auth hook.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/internal/app/app.go(2 hunks)ui/src/lib/yjs/useYjsSetup.ts(4 hunks)
🧰 Additional context used
🪛 GitHub Check: ci-ui / ci
ui/src/lib/yjs/useYjsSetup.ts
[warning] 6-6:
@flow/lib/auth import should occur after import of @flow/global-constants
🔇 Additional comments (1)
ui/src/lib/yjs/useYjsSetup.ts (1)
36-62: 🛠️ Refactor suggestionImprove WebSocket connection resilience.
The WebSocket connection setup needs better error handling and reconnection strategy:
- No error handling for WebSocket connection failures
- No cleanup if token retrieval fails
- Missing reconnection strategy for connection drops
(async () => { try { const token = await getAccessToken(); if (!token) { console.error('Failed to obtain authentication token'); return; } yWebSocketProvider = new WebsocketProvider( websocket, `${projectId}:${workflowId}`, yDoc, { params: { token }, + connect: true, + maxBackoffTime: 5000, }, ); + yWebSocketProvider.on('connection-error', error => { + console.error('WebSocket connection error:', error); + }); + + yWebSocketProvider.on('connection-close', () => { + console.warn('WebSocket connection closed, attempting to reconnect...'); + }); + yWebSocketProvider.once("sync", () => { if (yWorkflows.length === 0) { yDoc.transact(() => { const yWorkflow = yWorkflowConstructor( DEFAULT_ENTRY_GRAPH_ID, "Main Workflow", ); yWorkflows.push([yWorkflow]); }); } setIsSynced(true); }); } catch (error) { console.error('Failed to establish WebSocket connection:', error); + setIsSynced(false); } })();Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/docker-compose.yml(0 hunks)api/internal/app/app.go(2 hunks)websocket/docker-compose.yml(1 hunks)
💤 Files with no reviewable changes (1)
- api/docker-compose.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- api/internal/app/app.go
🔇 Additional comments (5)
websocket/docker-compose.yml (5)
7-7: Update WebSocket port to 8080 for consistency.The port should be changed to 8080 to align with existing benchmark code, as previously discussed.
- - "8000:8000" + - "8080:8080"
14-17: Enhance Redis configuration for production readiness.The Redis configuration needs improvements for security and reliability.
redis: image: redis:7-alpine ports: - "6379:6379" + command: redis-server --requirepass ${REDIS_PASSWORD} + environment: + - REDIS_PASSWORD=${REDIS_PASSWORD} + volumes: + - redis-data:/data + deploy: + resources: + limits: + memory: 1G
19-23: Address security and versioning concerns in fake-gcs configuration.The configuration needs improvements for security and stability.
fake-gcs: - image: fsouza/fake-gcs-server + image: fsouza/fake-gcs-server:v1.47.2 ports: - "4443:4443" - command: ["-scheme", "http"] + command: ["-scheme", "https", "-public-host", "localhost:4443"]
1-28: Verify integration points for yrs-warp architecture.The Docker Compose configuration supports the new yrs-warp architecture with Redis for real-time synchronization and GCS for persistence. However, please verify:
- Network configuration between services
- Authentication flow
- Data persistence strategy
✅ Verification successful
Architecture verification completed successfully
The integration points for the yrs-warp architecture are properly configured:
- Redis connectivity is correctly set up for real-time synchronization
- Authentication is implemented with proper token verification
- Service dependencies and network configuration are well-defined in the docker-compose setup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for network configuration and authentication setup echo "Checking service network configuration:" rg -g '!docker-compose.yml' "redis://" ./websocket echo -e "\nChecking authentication setup:" rg -g '!docker-compose.yml' "auth|authenticate|jwt" ./websocketLength of output: 3226
8-9: Verify required environment variables for yrs-warp integration.Since this PR refactors to use yrs-warp, ensure the
.envfile contains all necessary configuration variables.✅ Verification successful
Environment variables for yrs-warp integration are properly configured with defaults
The websocket service uses environment variables with sensible defaults for development. You can override them in the
.envfile if needed:
REEARTH_FLOW_REDIS_URL(default: "redis://127.0.0.1:6379")REEARTH_FLOW_REDIS_TTL(default: 3600)REEARTH_FLOW_GCS_BUCKET_NAME(default: "yrs-dev")REEARTH_FLOW_GCS_ENDPOINT(default: "http://localhost:4443")REEARTH_FLOW_APP_ENV(default: "development")REEARTH_FLOW_ORIGINS(default: "http://localhost:3000")When auth feature is enabled:
REEARTH_FLOW_AUTH_URL(default: "http://localhost:8080/api/verify/ws-token")REEARTH_FLOW_AUTH_TIMEOUT_MS(default: 5000)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if .env file exists and contains required variables if [ -f websocket/.env ]; then echo "Required variables for yrs-warp:" grep -E "^(REDIS_URL|GCS_BUCKET|GCS_ENDPOINT|AUTH_SECRET)=" websocket/.env || echo "Missing required variables" else echo ".env file not found" fiLength of output: 76
Script:
#!/bin/bash # Look for example env files and documentation echo "=== Searching for example env files ===" fd -t f -e example -e sample .env echo -e "\n=== Checking README files for env documentation ===" fd -t f README.md -x cat {} echo -e "\n=== Searching for environment variable usage in code ===" rg -l "process\.env\." websocket/Length of output: 13571
Script:
#!/bin/bash # Search for Rust environment variable patterns and config echo "=== Searching for Rust env var patterns ===" rg -l "std::env::(var|var_os)" websocket/ echo -e "\n=== Searching for config structs and environment documentation ===" rg -i -l "(struct\s+Config|env.*variables?)" websocket/ echo -e "\n=== Checking Cargo.toml for dependencies that might need env vars ===" cat websocket/Cargo.tomlLength of output: 2335
Script:
#!/bin/bash # Check configuration file content echo "=== Configuration file content ===" cat websocket/src/conf.rs # Also check for any example configurations echo -e "\n=== Looking for example configurations ===" fd -t f -e example -e sample -e template conf websocket/Length of output: 5308
kasugamirai
force-pushed
the
feat/websocket-yrs
branch
from
2cae2d9 to
86ad707
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
websocket/output/run.sh (1)
1-1: 🛠️ Refactor suggestionEnhance script robustness by adding a shebang and error handling.
The current one-line script (
./main) assumes the correct shell environment and that themainbinary exists and is executable. To ensure consistent behavior across environments and improve reliability, please add a shebang (e.g.,#!/bin/bash), enable error exit withset -e, and verify that the./mainbinary exists and is executable before running it.This suggestion closely mirrors previous recommendations, so it’s tagged as a duplicate comment.
+#!/bin/bash +set -e +if [ ! -x "./main" ]; then + echo "Error: main binary not found or not executable" + exit 1 +fi + ./main🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
websocket/script/run.sh (1)
1-1: 🛠️ Refactor suggestionEnhance Script Structure with Shebang and Robust Error Handling
The current script only contains the command
./main, which lacks a shebang and proper error handling. This can lead to issues in diverse execution environments (as noted by Shellcheck SC2148). It is highly recommended to adopt a more robust script template that includes a shebang, strict mode (set -euo pipefail), and validations to ensure that the binary exists and is executable.Consider applying the following diff to improve the script’s reliability:
-./main +#!/bin/bash +set -euo pipefail + +# Get the directory where the script is located +SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" +BINARY_PATH="${SCRIPT_DIR}/../target/debug/main" + +# Check if binary exists +if [ ! -f "$BINARY_PATH" ]; then + echo "Error: Binary not found at $BINARY_PATH" + echo "Please build the project first using 'cargo build'" + exit 1 +fi + +# Check if binary is executable +if [ ! -x "$BINARY_PATH" ]; then + echo "Error: Binary is not executable" + chmod +x "$BINARY_PATH" +fi + +# Execute the binary +exec "$BINARY_PATH"This structure will ensure that the script fails fast on errors and improves overall maintainability.
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
ui/src/lib/yjs/useYjsSetup.ts (1)
36-47:⚠️ Potential issueAdd error handling for token retrieval.
The token retrieval should include error handling to gracefully handle authentication failures.
(async () => { - const token = await getAccessToken(); - yWebSocketProvider = new WebsocketProvider( - websocket, - `${projectId}:${workflowId}`, - yDoc, - { - params: { - token, - }, - }, - ); + try { + const token = await getAccessToken(); + if (!token) { + console.error('Failed to obtain authentication token'); + return; + } + yWebSocketProvider = new WebsocketProvider( + websocket, + `${projectId}:${workflowId}`, + yDoc, + { + params: { + token, + }, + }, + ); + } catch (error) { + console.error('Authentication failed:', error); + }
🧹 Nitpick comments (5)
websocket/output/conf.yaml (1)
6-6: Fix YAML Lint Warning: Newline at EOFYAMLlint has flagged that there is no newline at the end of the file (line 6). Please add a newline character at the end of the file to adhere to the linting standards and avoid potential issues with tooling.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 6-6: no new line character at the end of file
(new-line-at-end-of-file)
ui/src/lib/yjs/useYjsSetup.ts (1)
6-6: Reorder imports according to conventions.Move the
@flow/lib/authimport after@flow/global-constantsto maintain consistent import ordering.-import { useAuth } from "@flow/lib/auth"; import { config } from "@flow/config"; import { DEFAULT_ENTRY_GRAPH_ID } from "@flow/global-constants"; +import { useAuth } from "@flow/lib/auth";🧰 Tools
🪛 GitHub Check: ci-ui / ci
[warning] 6-6:
@flow/lib/authimport should occur after import of@flow/global-constantswebsocket/benches/readme.adoc (1)
1-111: Comprehensive Benchmark Suite Documentation with Room for EnhancementThis documentation is well detailed—it outlines the purpose, execution steps, and benchmarks (connection tests, document operations, concurrent operations, etc.) effectively. For added clarity, consider including explicit performance baselines, configuration options (such as environment variables to tweak sample sizes or delays), and known performance limitations. These additions can further assist users in understanding and interpreting benchmark results.
websocket/benches/server_bench.rs (2)
15-42: Enhance error handling in verify_message_reception.The function could benefit from more robust error handling:
- Add timeout handling for the message application
- Include more context in error messages
Consider this improved implementation:
async fn verify_message_reception( read: &mut futures_util::stream::SplitStream< tokio_tungstenite::WebSocketStream< tokio_tungstenite::MaybeTlsStream<tokio::net::TcpStream>, >, >, expected_doc: &Doc, ) -> Result<(), Box<dyn std::error::Error>> { let msg = tokio::time::timeout(TIMEOUT_DURATION, read.next()) .await - .map_err(|_| "Timeout waiting for response")? + .map_err(|_| format!("Timeout after {:?} waiting for response", TIMEOUT_DURATION))? .ok_or("Connection closed unexpectedly")? .map_err(|e| format!("WebSocket error: {}", e))?; match msg { Message::Binary(data) => { // Try to apply the update to verify it's valid CRDT data - let update = - Update::decode_v1(&data).map_err(|e| format!("Invalid CRDT update: {}", e))?; - let mut txn = expected_doc.transact_mut(); - txn.apply_update(update) - .map_err(|e| format!("Failed to apply update: {}", e))?; + let update = Update::decode_v1(&data) + .map_err(|e| format!("Invalid CRDT update (len: {}): {}", data.len(), e))?; + tokio::time::timeout( + TIMEOUT_DURATION, + tokio::task::spawn_blocking(move || { + let mut txn = expected_doc.transact_mut(); + txn.apply_update(update) + }), + ) + .await + .map_err(|_| "Timeout applying update")? + .map_err(|e| format!("Task failed: {}", e))? + .map_err(|e| format!("Failed to apply update: {}", e))?; Ok(()) } Message::Close(_) => Err("Received unexpected close frame".into()), _ => Err("Received unexpected message type".into()), } }
480-486: Adjust benchmark configuration for more accurate results.The current configuration might not provide statistically significant results:
- Sample size of 10 is quite small for benchmarks
- Warm-up time of 500ms might not be sufficient
Consider these improvements:
fn configure_criterion() -> Criterion { Criterion::default() - .sample_size(10) - .measurement_time(Duration::from_secs(1)) - .warm_up_time(Duration::from_millis(500)) + .sample_size(50) // Increased for better statistical significance + .measurement_time(Duration::from_secs(2)) + .warm_up_time(Duration::from_secs(1)) .output_directory(std::path::Path::new("target/criterion")) + .confidence_level(0.95) // Add explicit confidence level }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
websocket/.DS_Storeis excluded by!**/.DS_Storewebsocket/Cargo.lockis excluded by!**/*.lock,!**/*.lockwebsocket/README.mdis excluded by!**/*.md
📒 Files selected for processing (82)
api/docker-compose.yml(0 hunks)api/internal/app/app.go(2 hunks)ui/src/lib/yjs/useYjsSetup.ts(5 hunks)websocket/.dockerignore(0 hunks)websocket/.env-sample(1 hunks)websocket/.gitignore(1 hunks)websocket/.taplo.toml(0 hunks)websocket/Cargo.toml(1 hunks)websocket/Dockerfile(1 hunks)websocket/LICENSE(1 hunks)websocket/Makefile.toml(0 hunks)websocket/README.adoc(1 hunks)websocket/Redis.Dockerfile(0 hunks)websocket/Websocket.Dockerfile(0 hunks)websocket/app/Cargo.toml(0 hunks)websocket/app/examples/readme.adoc(0 hunks)websocket/app/examples/room_client.rs(0 hunks)websocket/app/examples/session_client.rs(0 hunks)websocket/app/readme.adoc(0 hunks)websocket/app/src/bin/main.rs(0 hunks)websocket/app/src/config.rs(0 hunks)websocket/app/src/errors.rs(0 hunks)websocket/app/src/handlers/cleanup.rs(0 hunks)websocket/app/src/handlers/heartbeat.rs(0 hunks)websocket/app/src/handlers/message_handler.rs(0 hunks)websocket/app/src/handlers/mod.rs(0 hunks)websocket/app/src/handlers/room_handler.rs(0 hunks)websocket/app/src/handlers/socket_handler.rs(0 hunks)websocket/app/src/handlers/types.rs(0 hunks)websocket/app/src/lib.rs(0 hunks)websocket/app/src/middleware/mod.rs(0 hunks)websocket/app/src/middleware/trace.rs(0 hunks)websocket/app/src/room.rs(0 hunks)websocket/app/src/routes.rs(0 hunks)websocket/app/src/state.rs(0 hunks)websocket/benches/readme.adoc(1 hunks)websocket/benches/server_bench.rs(1 hunks)websocket/build.sh(1 hunks)websocket/clippy.toml(0 hunks)websocket/conf/conf.yaml(0 hunks)websocket/config/config.toml(0 hunks)websocket/crates/infra/Cargo.toml(0 hunks)websocket/crates/infra/src/lib.rs(0 hunks)websocket/crates/infra/src/persistence/editing_session.rs(0 hunks)websocket/crates/infra/src/persistence/event_handler.rs(0 hunks)websocket/crates/infra/src/persistence/gcs/gcs_client.rs(0 hunks)websocket/crates/infra/src/persistence/gcs/mod.rs(0 hunks)websocket/crates/infra/src/persistence/local_storage/local_client.rs(0 hunks)websocket/crates/infra/src/persistence/local_storage/mod.rs(0 hunks)websocket/crates/infra/src/persistence/mod.rs(0 hunks)websocket/crates/infra/src/persistence/project_repository.rs(0 hunks)websocket/crates/infra/src/persistence/redis/connection.rs(0 hunks)websocket/crates/infra/src/persistence/redis/default_key_manager.rs(0 hunks)websocket/crates/infra/src/persistence/redis/errors.rs(0 hunks)websocket/crates/infra/src/persistence/redis/flow_project_lock.rs(0 hunks)websocket/crates/infra/src/persistence/redis/flow_project_redis_data_manager.rs(0 hunks)websocket/crates/infra/src/persistence/redis/keys.rs(0 hunks)websocket/crates/infra/src/persistence/redis/mod.rs(0 hunks)websocket/crates/infra/src/persistence/redis/types.rs(0 hunks)websocket/crates/infra/src/persistence/redis/updates.rs(0 hunks)websocket/crates/infra/src/persistence/repository.rs(0 hunks)websocket/crates/infra/src/types/mod.rs(0 hunks)websocket/crates/infra/src/types/project.rs(0 hunks)websocket/crates/infra/src/types/snapshot.rs(0 hunks)websocket/crates/infra/src/types/user.rs(0 hunks)websocket/crates/infra/src/types/workspace.rs(0 hunks)websocket/crates/infra/src/utils.rs(0 hunks)websocket/crates/services/Cargo.toml(0 hunks)websocket/crates/services/examples/edit_session_service.rs(0 hunks)websocket/crates/services/examples/project_service.rs(0 hunks)websocket/crates/services/examples/readme.adoc(0 hunks)websocket/crates/services/src/error.rs(0 hunks)websocket/crates/services/src/lib.rs(0 hunks)websocket/crates/services/src/manage_project_edit_session.rs(0 hunks)websocket/crates/services/src/project.rs(0 hunks)websocket/crates/services/src/types.rs(0 hunks)websocket/docker-compose.yml(1 hunks)websocket/output/conf.yaml(1 hunks)websocket/output/run.sh(1 hunks)websocket/redis.conf(0 hunks)websocket/rustfmt.toml(0 hunks)websocket/script/run.sh(1 hunks)
⛔ Files not processed due to max files limit (15)
- websocket/src/auth/mod.rs
- websocket/src/bin/main.rs
- websocket/src/broadcast/group.rs
- websocket/src/broadcast/mod.rs
- websocket/src/broadcast/pool.rs
- websocket/src/conf.rs
- websocket/src/conn.rs
- websocket/src/lib.rs
- websocket/src/storage/gcs/mod.rs
- websocket/src/storage/kv/error.rs
- websocket/src/storage/kv/keys.rs
- websocket/src/storage/kv/mod.rs
- websocket/src/storage/mod.rs
- websocket/src/storage/sqlite/mod.rs
- websocket/src/ws.rs
💤 Files with no reviewable changes (67)
- websocket/app/readme.adoc
- websocket/crates/infra/src/persistence/gcs/mod.rs
- websocket/clippy.toml
- websocket/.dockerignore
- websocket/config/config.toml
- api/docker-compose.yml
- websocket/crates/infra/src/utils.rs
- websocket/rustfmt.toml
- websocket/crates/services/examples/readme.adoc
- websocket/app/src/errors.rs
- websocket/app/src/middleware/trace.rs
- websocket/crates/infra/src/persistence/event_handler.rs
- websocket/crates/infra/src/persistence/local_storage/mod.rs
- websocket/redis.conf
- websocket/Redis.Dockerfile
- websocket/conf/conf.yaml
- websocket/crates/infra/Cargo.toml
- websocket/crates/infra/src/lib.rs
- websocket/crates/services/src/project.rs
- websocket/app/src/bin/main.rs
- websocket/crates/services/src/error.rs
- websocket/app/examples/readme.adoc
- websocket/crates/infra/src/types/project.rs
- websocket/app/src/handlers/types.rs
- websocket/crates/infra/src/persistence/editing_session.rs
- websocket/app/examples/room_client.rs
- websocket/crates/services/src/types.rs
- websocket/.taplo.toml
- websocket/crates/infra/src/types/mod.rs
- websocket/Websocket.Dockerfile
- websocket/app/src/handlers/cleanup.rs
- websocket/crates/infra/src/persistence/repository.rs
- websocket/crates/services/examples/edit_session_service.rs
- websocket/app/Cargo.toml
- websocket/app/src/handlers/mod.rs
- websocket/crates/services/examples/project_service.rs
- websocket/crates/infra/src/persistence/redis/default_key_manager.rs
- websocket/app/src/routes.rs
- websocket/crates/infra/src/persistence/redis/errors.rs
- websocket/app/src/middleware/mod.rs
- websocket/crates/infra/src/persistence/redis/keys.rs
- websocket/crates/infra/src/persistence/redis/connection.rs
- websocket/app/src/config.rs
- websocket/app/src/handlers/heartbeat.rs
- websocket/crates/infra/src/persistence/project_repository.rs
- websocket/app/src/handlers/socket_handler.rs
- websocket/app/examples/session_client.rs
- websocket/app/src/lib.rs
- websocket/crates/infra/src/persistence/mod.rs
- websocket/crates/infra/src/persistence/redis/mod.rs
- websocket/crates/infra/src/types/user.rs
- websocket/Makefile.toml
- websocket/crates/infra/src/persistence/redis/types.rs
- websocket/app/src/handlers/room_handler.rs
- websocket/app/src/handlers/message_handler.rs
- websocket/crates/services/src/lib.rs
- websocket/crates/infra/src/persistence/redis/updates.rs
- websocket/app/src/room.rs
- websocket/crates/infra/src/persistence/redis/flow_project_redis_data_manager.rs
- websocket/crates/infra/src/types/snapshot.rs
- websocket/crates/infra/src/persistence/redis/flow_project_lock.rs
- websocket/crates/services/src/manage_project_edit_session.rs
- websocket/app/src/state.rs
- websocket/crates/infra/src/persistence/local_storage/local_client.rs
- websocket/crates/infra/src/persistence/gcs/gcs_client.rs
- websocket/crates/infra/src/types/workspace.rs
- websocket/crates/services/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (6)
- websocket/LICENSE
- websocket/.gitignore
- websocket/Dockerfile
- api/internal/app/app.go
- websocket/README.adoc
- websocket/.env-sample
🧰 Additional context used
🪛 GitHub Check: ci-ui / ci
ui/src/lib/yjs/useYjsSetup.ts
[warning] 6-6:
@flow/lib/auth import should occur after import of @flow/global-constants
🪛 GitHub Actions: ci
ui/src/lib/yjs/useYjsSetup.ts
[warning] 1-1: Code style issues found in the above file. Run Prettier with --write to fix.
🪛 Shellcheck (0.10.0)
websocket/build.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
websocket/output/run.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
websocket/script/run.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🪛 YAMLlint (1.35.1)
websocket/output/conf.yaml
[error] 6-6: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - reearth-flow
- GitHub Check: Header rules - reearth-flow
- GitHub Check: Pages changed - reearth-flow
🔇 Additional comments (17)
websocket/output/conf.yaml (2)
1-3: Enhance Redis Configuration FlexibilityConsider using environment variables for the Redis URL. This increases flexibility and security by allowing different configurations for various deployment environments. For example, instead of hardcoding the URL, you could use:
- url: redis://127.0.0.1:6379 + url: ${REEARTH_FLOW_REDIS_URL:-redis://127.0.0.1:6379}
4-6: Make GCS Configuration Environment-AwareSimilar to the Redis settings, parameterizing the GCS configurations with environment variables can improve security and portability. Instead of using fixed values, consider adopting:
- bucket_name: yrs-dev - endpoint: http://localhost:4443 + bucket_name: ${REEARTH_FLOW_GCS_BUCKET:-yrs-dev} + endpoint: ${REEARTH_FLOW_GCS_ENDPOINT:-http://localhost:4443}This change aligns with prior recommendations and prepares the system for environments beyond local development.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 6-6: no new line character at the end of file
(new-line-at-end-of-file)
ui/src/lib/yjs/useYjsSetup.ts (2)
73-76: LGTM!The cleanup logic is well-implemented, and the dependency array correctly includes
getAccessToken.
82-94: LGTM!The UndoManager implementation follows best practices with proper cleanup and optimal capture timeout settings.
websocket/build.sh (1)
1-6: Enhance Script Robustness with Shebang and Error HandlingThe build script currently lacks a shebang and robust error handling. For consistency with Shellcheck recommendations (SC2148) and to prevent unintended behavior, please add a shebang (e.g.,
#!/bin/bash) and consider enabling error exit withset -e. Optionally, verify the existence of source directories/files before attempting to copy them.+#!/bin/bash +set -e mkdir output -cp script/* output 2>/dev/null -cp -r src/conf.yaml output/ 2>/dev/null +if [ -d "script" ]; then + cp script/* output 2>/dev/null +fi +if [ -f "src/conf.yaml" ]; then + cp -r src/conf.yaml output/ 2>/dev/null +fi cargo build --bin main --release cp target/release/main output chmod +x output/*🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
websocket/Cargo.toml (4)
1-11: Verify Package Metadata ConsistencyThe package metadata has been updated for the yrs-warp integration. Please confirm that values such as the homepage, repository, and authors align with your current project objectives. If any parts need to reflect integration with Re:Earth Flow or additional context, consider revising these fields accordingly.
19-51: Review Dependency UpdatesThe dependencies now include key libraries (e.g.,
yrs,axum,tower-http) essential for the new WebSocket implementation. Please verify that these version constraints are compatible and meet your security and performance requirements. Also, ensure that any potential TLS or configuration settings for dependencies likeredisare handled elsewhere.
52-59: Confirm Dev Dependencies CompatibilityThe development dependencies appear appropriately set up. Pay attention to using multiple specifications for
tokio(in both regular and dev dependencies) to avoid potential conflicts. A brief check or note in documentation might help future maintainers.
60-63: Validate Benchmark ConfigurationThe benchmark section (named
server_benchwith harness disabled) looks correct. Ensure that this configuration aligns with your benchmarking strategy and that any corresponding documentation (e.g., in the benchmarks’ README) clearly explains how the benchmarks are set up and executed.websocket/docker-compose.yml (6)
5-5: Ensure Dockerfile Reference is Correct
Line 5 now specifies a genericDockerfileinstead of a service-specific one. Verify that the generic Dockerfile accommodates the unique build requirements of the websocket service.
7-9: Confirm Port Mapping and Environment File Configuration
Lines 7–9 expose the websocket service on"8000:8000"and introduce an.envfile. Please check that the port is intentionally retained (especially in light of previous discussions about updating ports) and that the.envfile exists with all necessary environment variables for proper service operation.
10-12: Review Dependency Configuration for Services
Lines 11–12 now include dependencies on bothredisandfake-gcs. Ensure that these services are reliably available before the websocket service starts and consider adding health checks if deploying to production.
14-18: Evaluate Redis Service Configuration for Production Use
Lines 14–18 define the Redis service using theredis:7-alpineimage and expose it on port"6379:6379". For production deployments, consider securing the Redis instance (for example, limiting external access or using password protection). If this configuration is intended solely for development, it may be acceptable.
27-29: Validate Volume Configuration for Persistent Storage
Lines 27–29 declare thegcs-datavolume for use with the fake GCS service. Confirm that this volume is appropriately mounted to persist data as intended across container restarts.
19-25: 🛠️ Refactor suggestion
⚠️ Potential issueCheck Fake-GCS Service Setup and Security Settings
Lines 19–25 configure thefake-gcsservice with the imagefsouza/fake-gcs-server, exposing port"4443:4443", and setting the command to use["-scheme", "http"]. Since port 4443 typically implies HTTPS, it is advisable to update the command to use HTTPS and possibly specify the public host. For example, consider:- command: ["-scheme", "http"] + command: ["-scheme", "https", "-public-host", "localhost:4443"]This change would align the configuration with best practices for secure communications.
Likely invalid or redundant comment.
websocket/benches/server_bench.rs (2)
182-242: Improve resource management in concurrent benchmarks.The concurrent benchmark could be improved in several ways:
- Use a connection pool for better resource management
- Implement graceful shutdown for error cases
- Add timeouts for concurrent operations
244-314:⚠️ Potential issueFix potential memory leak in broadcast benchmark.
The broadcast benchmark doesn't properly clean up resources when errors occur.
Apply this fix:
fn bench_broadcast(c: &mut Criterion) { let rt = Builder::new_current_thread().enable_all().build().unwrap(); const NUM_RECEIVERS: usize = 3; let mut group = c.benchmark_group("broadcast"); group.sample_size(10); group.bench_function("broadcast", |b| { b.iter(|| { rt.block_on(async { + let mut cleanup_tasks = Vec::with_capacity(NUM_RECEIVERS + 1); // Create multiple receiver connections let mut receivers = Vec::with_capacity(NUM_RECEIVERS); let mut receiver_reads = Vec::with_capacity(NUM_RECEIVERS); for _ in 0..NUM_RECEIVERS { let ws_stream = connect_client("bench-doc-broadcast").await; let (write, read) = ws_stream.split(); receivers.push(write); receiver_reads.push(read); + cleanup_tasks.push(tokio::spawn(async move { + sleep(Duration::from_secs(5)).await; // Safety timeout + Ok::<_, Box<dyn std::error::Error>>(()) + })); } // Rest of the benchmark code... + // Ensure all resources are cleaned up + for task in cleanup_tasks { + if let Err(e) = task.await { + tracing::error!("Cleanup task failed: {}", e); + } + } }); }) }); group.finish(); }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/api/.env.example (2)
18-19: Clarify REEARTH_FLOW_GCS_PUBLICATIONCACHECONTROL Value
The variableREEARTH_FLOW_GCS_PUBLICATIONCACHECONTROLis currently set to an empty value. If this is intentional (for example, to allow overriding in different environments), please add an inline comment or documentation clarifying its purpose. Otherwise, consider providing a sensible default value.
26-27: Validate gRPC Server Endpoint Configuration
The new variableREEARTH_FLOW_WEBSOCKET_GRPC_SERVER=localhost:50051is introduced to configure the gRPC server for document management. Please ensure that this endpoint is correctly referenced in your gRPC server initialization and that it meets your environment’s security and networking requirements.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/api/.env.example(1 hunks)
🔇 Additional comments (1)
server/api/.env.example (1)
20-25: Review WebSocket Configuration Variables
A new configuration block for WebSocket-related settings has been added. In particular, the variableREEARTH_FLOW_WEBSOCKET_GCS_ENDPOINTis set tohttp://localhost:4443. If this endpoint is exposed publicly or used in a production-like environment, consider switching to a secure protocol (for example, usingwss://orhttps://as applicable). Also, verify that these values (bucket name, Redis URL, TTL) align with your deployment and security requirements.Suggested change (if needed):
-REEARTH_FLOW_WEBSOCKET_GCS_ENDPOINT=http://localhost:4443 +REEARTH_FLOW_WEBSOCKET_GCS_ENDPOINT=wss://localhost:4443
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/ci_api.yml (3)
37-40: Rust Toolchain Setup - Pin the Action Version
The step usingdtolnay/rust-toolchain@masterinstalls the specified Rust version. Using themasterbranch can introduce unexpected breaking changes over time. Consider pinning to a stable release or a specific commit hash to ensure consistent builds.
45-52: Build WebSocket Library Process
The "Build WebSocket Library" step compiles the Rust library, creates the designated target directory, and copies the output usingcp target/release/libwebsocket.* ../target/release/. Ensure that this wildcard correctly captures all necessary library files generated by Cargo and that it works consistently across different environments. If your project ever changes the naming convention, you might need to update this command accordingly.
58-60: Pre-Test Debug Command in Test Step
The inclusion ofls -la ${{ github.workspace }}/server/target/release/before running Go tests provides useful visibility into build artifacts. Once confirmed, consider removing or conditionally running this command to avoid unnecessary log noise in production CI runs.server/api/Dockerfile (1)
5-16: Enhanced Package Installation.
Theapk addcommand now installs additional packages (e.g.,protoc,protobuf-dev,curl,rust,cargo,openssl-dev,pkgconfig) that are required for Rust builds and for generating Go code from proto definitions. Consider whether version pinning for these packages is needed for reproducible builds.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci_api.yml(2 hunks)server/api/Dockerfile(1 hunks)
🔇 Additional comments (23)
.github/workflows/ci_api.yml (2)
4-6: New Environment Variable for Rust Version
The new environment variableRUST_VERSION: "1.75"is added alongsideGO_VERSION. Please verify that version "1.75" is compatible with your Rust project requirements and the subsequent toolchain setup.
41-44: Caching Rust Dependencies Configuration
The new caching step usingSwatinem/rust-cache@v2with the workspace mapping"server/websocket -> target"is a useful optimization. Please verify that the workspace syntax correctly identifies the cache paths and that cache keys are updated appropriately when dependencies change.server/api/Dockerfile (21)
1-1: Updated Base Image.
The base image is now updated togolang:1.24-alpine, which helps keep up with the latest Golang releases and security improvements.
20-24: Updated File Copy Commands for Rust Components.
The Dockerfile now copies crucial Rust project files (such asCargo.toml,Cargo.lock, source files, benchmarks, build scripts, and proto definitions) fromserver/websocket/into the build context. Verify that the destination directory (/reearth-flow/server/) correctly reflects the later build commands.
26-27: Rust Build Command.
Switching to the/reearth-flow/serverdirectory and runningcargo build --releaseappropriately compiles the Rust components. Ensure that any subsequent references to the build artifacts (e.g., the shared library) match the generated paths.
28-28: Resetting to Project Root.
Returning to the/reearth-flowdirectory after the Rust build is appropriate to prepare for the Go build and further file operations.
30-32: Symlink and ldconfig Setup.
Creating a symlink forlibwebsocket.soin/usr/local/liband runningldconfighelps ensure that dynamic linking will locate the shared library. On Alpine Linux, please verify thatldconfigbehaves as expected with musl-based systems.
34-35: Protobuf and gRPC Tools Installation.
Installingprotoc-gen-goandprotoc-gen-go-grpcwith fixed versions helps maintain consistency in the generated code. This step is clear and necessary.
37-37: Copying Go Module Files.
Copying thego.mod,go.sum, andmain.gofiles into the build context ensures that the dependency definitions are up to date before building the Go binary.
38-38: Dependency Download.
Executinggo mod downloadright after copying the module files properly fetches all required Go dependencies.
40-44: Copying API Codebase Files.
The inclusion of API source directories (cmd,pkg,internal, andproto) ensures that the complete Go application is available for building. Confirm that these directories match the expected project structure.
45-47: Protobuf Code Generation.
Theprotoccommand correctly generates Go source files from the proto definitions using the--go_outand--go-grpc_outflags while preserving source-relative paths.
49-49: Setting Library Path in Build Stage.
The environment variableLD_LIBRARY_PATHis set to/reearth-flow/server/target/releaseso that the build process can locatelibwebsocket.so. This is important for CGO-enabled builds that rely on dynamic libraries.
50-50: Go Application Build Command.
Runninggo buildwithCGO_ENABLED=1and customRUSTFLAGSties the Go application with the Rust component seamlessly. Ensure that the final binary incorporates the correct build version and stripped debug information.
52-52: Final Stage Base Image Update.
Transitioning toFROM alpine:latestfor the final stage yields a minimal runtime image that still includes necessary updates like CA certificates.
54-58: Installing Runtime Dependencies.
The final stage installs the runtime packages (ca-certificates,libc6-compat,libgcc,libstdc++) ensuring that both SSL and C library dependencies are met.
60-60: Copying CA Certificates.
Duplicating the CA certificates from the build image guarantees that secure connections are possible in the runtime container.
61-61: Copying the Go Executable.
The builtreearth-flowbinary is copied into the final image, ensuring that the main application is included.
62-62: Copying Rust Shared Library.
Bringing over thelibwebsocket.sofile to/reearth-flow/target/release/in the final image ensures that the Go binary can locate and link to the Rust library at runtime.
63-63: Copying UI Assets.
The UI assets are copied into the final image under/reearth-flow/ui, ensuring they are available for serving.
65-65: Final Working Directory.
SettingWORKDIR /reearth-flowin the final stage establishes the application root; this is critical for correctly resolving relative paths at runtime.
67-67: Final Library Path Setup.
Resetting theLD_LIBRARY_PATHto/reearth-flow/target/releaseensures that the shared libraries required by the Go binary are found at runtime.
69-69: Command to Run the Application.
The finalCMD ["./reearth-flow"]correctly defines the entrypoint for the container, allowing the application to launch as expected when the container starts.
…t service - Delete auto-generated document_grpc.pb.go - Remove document.pb.go - Delete document.proto definition file
- Add fallback for different library file extensions (so/dylib) - Add debug listing of release target directory
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/ci_api.yml (4)
37-40: Rust Toolchain Setup Action
The new "Install Rust" step correctly leverages thedtolnay/rust-toolchainaction with the specified toolchain via the environment variable. However, consider pinning the action to a stable version rather than using themasterbranch to prevent potential breaking changes in the future.
45-48: Installing Protoc Compiler
Introducing the "Install Protoc" step ensures that the protobuf compiler is available during the workflow. It would be beneficial to verify that the installed version ofprotobuf-compileris compatible with your.protofiles, as version discrepancies can occasionally introduce compatibility issues.
49-56: Building and Copying the WebSocket Library
The "Build WebSocket Library" step correctly builds the Rust component and attempts to copy the generated library files (with different extensions for Linux and macOS) to a shared output directory. The use of brace expansion with an OR operator (||) could be fragile if file naming conventions change. Consider splitting the copy command into explicit conditional branches based on the detected operating system or file existence for greater robustness.
63-65: Pre-test Artifact Listing for Debugging
The added command to list the contents of${{ github.workspace }}/server/target/release/improves build transparency and debugging. Ensure that this level of logging is intended for long-term usage, or consider removing it after confirming the successful artifact build to keep the CI logs concise.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci_api.yml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci-api / ci-api-test
🔇 Additional comments (2)
.github/workflows/ci_api.yml (2)
6-6: New Rust Version Environment Variable
The addition of theRUST_VERSION: "1.85"environment variable streamlines the Rust toolchain configuration in your workflow. Please verify that version 1.85 is fully compatible with your Rust components and dependencies.
41-44: Caching Rust Dependencies
The "Cache Rust dependencies" step usingSwatinem/rust-cache@v2looks like a well-intentioned addition to speed up builds. Please double-check that the workspace notation"server/websocket -> target"correctly maps to your project’s caching structure and meets the action’s expected parameters.
- Use GitHub workspace variable for consistent path resolution - Add verbose build and directory listing for debugging - Improve library file copying with explicit workspace paths
…tory - Use custom target directory for Rust WebSocket library build - Simplify library file copying and path handling - Update directory listing and file copying commands
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
server/websocket/src/lib.rs (1)
29-42:⚠️ Potential issueConsider making auth token required when auth feature is enabled.
When the auth feature is enabled, the token field in both
AuthQueryandRollbackQuerystructs is optional due to#[serde(default)]. This could lead to security issues if authentication is bypassed.server/websocket/build.rs (1)
1-4: 🛠️ Refactor suggestionUse CARGO_MANIFEST_DIR for reliable proto file paths.
The current implementation uses relative paths which might not work correctly depending on the working directory during build.
Apply this diff to make the paths more reliable:
fn main() -> Result<(), Box<dyn std::error::Error>> { - tonic_build::configure().compile(&["proto/auth.proto"], &["proto"])?; + let manifest_dir = std::env::var("CARGO_MANIFEST_DIR").unwrap(); + tonic_build::configure().compile(&[&format!("{}/proto/auth.proto", manifest_dir)], &[&format!("{}/proto", manifest_dir)])?; Ok(()) }Let's verify the proto files exist:
#!/bin/bash # Description: Check if proto files exist in the expected location cd server/websocket if [ ! -f "proto/auth.proto" ]; then echo "Error: Proto file not found in expected location" exit 1 fi
🧹 Nitpick comments (3)
server/websocket/src/lib.rs (2)
18-23: Add documentation for the AwarenessRef type alias.The type alias nicely encapsulates the concurrent awareness type. Consider adding documentation to explain its purpose and usage.
+/// Thread-safe reference to a Y.js awareness instance. +/// Used for managing shared state across concurrent connections. pub type AwarenessRef = Arc<RwLock<yrs::sync::Awareness>>;
44-55: Add documentation for AppState variants.While the implementation is correct, adding documentation would help explain the purpose of each variant and when they're used.
+/// Application state when authentication is enabled. +/// Contains both the broadcast pool and authentication service. #[cfg(feature = "auth")] #[derive(Clone, Debug)] pub struct AppState { pub pool: Arc<BroadcastPool>, pub auth: Arc<AuthService>, } +/// Application state when authentication is disabled. +/// Contains only the broadcast pool. #[cfg(not(feature = "auth"))] #[derive(Clone, Debug)] pub struct AppState { pub pool: Arc<BroadcastPool>, }.github/workflows/ci_api.yml (1)
49-62: Clean up debug commands.The build steps include many debug echo and ls commands that make the workflow noisy and harder to read.
Apply this diff to clean up the debug commands:
- name: Build WebSocket Library run: | cd ${{ github.workspace }}/server/websocket - pwd - echo "Building websocket library..." cargo build --release --target-dir ../target --verbose - echo "Build completed, checking target directory..." - ls -la ../target - echo "Checking release directory..." - ls -la ../target/release/ mkdir -p ../target/release cp ../target/release/libwebsocket.{so,rlib} ../target/release/ || cp ../target/release/libwebsocket.{dylib,rlib} ../target/release/ - echo "Checking copied files..." - ls -la ../target/release/
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/ci_api.yml(2 hunks)server/websocket/build.rs(1 hunks)server/websocket/docker-compose.yml(1 hunks)server/websocket/src/bin/websocket.rs(1 hunks)server/websocket/src/lib.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- server/websocket/docker-compose.yml
- server/websocket/src/bin/websocket.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci-api / ci-api-test
🔇 Additional comments (4)
server/websocket/src/lib.rs (2)
1-17: LGTM! Well-structured module organization with proper concurrency primitives.The use of
ArcandRwLockfrom tokio shows good consideration for concurrent access patterns. The modular structure with clear separation of concerns (auth, broadcast, storage, etc.) enhances maintainability.
57-66: LGTM! Clear and well-organized public exports.The exports provide a clean public API surface, with appropriate conditional compilation for auth-related functionality.
.github/workflows/ci_api.yml (2)
37-44: LGTM!The Rust setup steps are correctly configured using appropriate actions and caching.
45-48: LGTM!The protobuf compiler installation steps are correctly configured.
| @@ -3,6 +3,7 @@ on: | |||
| workflow_call: | |||
| env: | |||
| GO_VERSION: "1.22" | |||
| RUST_VERSION: "1.85" | |||
There was a problem hiding this comment.
Fix the Rust version format.
The specified Rust version "1.85" is not valid. Rust versions follow the format "1.xx.y" (e.g., "1.75.0").
Apply this diff to fix the version format:
- RUST_VERSION: "1.85"
+ RUST_VERSION: "1.75.0"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RUST_VERSION: "1.85" | |
| RUST_VERSION: "1.75.0" |
- Remove redundant directory creation and file copying steps - Streamline CI workflow for WebSocket library build process
…I module - Replace error-level logging with debug-level logging in FFI error handling - Maintain logging context while reducing verbosity of error reporting
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
server/websocket/src/ffi.rs (4)
14-30: Use a single, shared Tokio runtime instead of creating one anew.
Repeatedly callingtokio::runtime::Runtime::new().unwrap()for each FFI function may introduce unnecessary overhead and complexity. A better approach is to initialize one runtime (e.g., ininit_tracingor a dedicated initializer) and reuse it to handle async tasks.fn init_tracing() { use tracing_subscriber::{fmt, EnvFilter}; static INIT: std::sync::Once = std::sync::Once::new(); INIT.call_once(|| { + // Possibly also initialize a global static tokio::Runtime here fmt() .with_env_filter( EnvFilter::from_default_env().add_directive("websocket=debug".parse().unwrap()), ) ...
32-59: Confirm error handling strategy forcreate_pool.
Currently, when creatingGcsStoreor the subsequent pool fails, the function returnsNone. This propagates an FFI return ofnull_mut(), which is a valid approach. However, consider whether a more descriptive error code or separate logging might help differentiate error states for debugging in downstream code.
90-232: Consider validating doc existence before loading.
Although the code checksOk(false)for a missing document (line 205), it might be helpful to handle partial or corrupt documents specifically, logging more details. This will improve observability in production.
385-396: Ensure matching calls tofree_string.
FFI consumers must callfree_stringexactly once for every non-null pointer returned from these Rust functions. Failing to do so may create a memory leak. Provide explicit documentation or usage examples to prevent misuse.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci_api.yml(2 hunks)server/websocket/src/ffi.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci-api / ci-api-test
🔇 Additional comments (4)
server/websocket/src/ffi.rs (1)
310-383: Double-check rollback concurrency.
Ensure that no other operations can concurrently modify the same document while rolling back. Although using a single store abstraction might implicitly handle it, you might need to add checks or locks if partial updates are possible..github/workflows/ci_api.yml (3)
6-6: Duplicate: fix the Rust version format.
The specified version"1.85"does not follow the typical Rust version format (e.g.,"1.75.0").- RUST_VERSION: "1.85" + RUST_VERSION: "1.75.0"
37-45: Validate pinned versions for reproducible builds.
Usingdtolnay/rust-toolchain@masterand installing system packages (Protoc) without a specific version can lead to non-deterministic results in CI. Pin exact version tags or package versions to improve reproducibility and guard against future breakage.Also applies to: 47-49, 50-58
65-72: Good job exporting the library path and listing build artifacts.
This helps diagnose build or linking issues before running tests. The approach here of checking the directory contents is straightforward and clear.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
server/websocket/src/bin/websocket.rs (2)
28-34: 🛠️ Refactor suggestionImplement robust error handling and retry logic for GCS initialization.
The current implementation could benefit from retry logic for GCS store initialization.
68-72: 🛠️ Refactor suggestionImplement graceful shutdown handling.
The server startup should handle graceful shutdown and provide more detailed error context.
🧹 Nitpick comments (7)
server/websocket/src/broadcast/pool.rs (2)
65-71: Prefer typed error handling rather than matching on string content.Using
if e.to_string().contains("not found")is fragile. Relying on the error message text can break if the message changes or if different localization is applied. Consider creating a custom error type or using typed error enums to identify “document not found” conditions more reliably.
98-111: Reevaluate the approach to cleaning up empty groups.The current
.retaincall removes entries in an O(N) scan across theDashMap. If the number of persisted documents grows significantly, this could become a performance bottleneck. Consider alternative cleanup strategies, such as segmenting groups by last-activity time or applying lazy cleanup at fewer intervals, to minimize performance overhead.server/websocket/src/bin/websocket.rs (1)
13-19: Consider environment-based logging configuration.The tracing configuration could be more flexible by allowing log level configuration through environment variables.
+ let log_level = std::env::var("LOG_LEVEL") + .map(|level| level.parse().unwrap_or(tracing::Level::INFO)) + .unwrap_or(tracing::Level::INFO); tracing_subscriber::fmt() - .with_max_level(tracing::Level::INFO) + .with_max_level(log_level) .with_file(true) .with_line_number(true) .init();server/websocket/src/ffi.rs (4)
38-39: Consider avoiding repeated runtime creation for better performance.Currently, multiple calls to
tokio::runtime::Runtime::new().unwrap()are spread across different functions, leading to the creation of a fresh runtime per invocation. This can hurt performance and increase complexity. Instead, consider creating a single shared runtime at startup or passing references to a shared runtime to these FFI functions.Below is an example design snippet that demonstrates how to store a global runtime reference:
+// In a global context (or a separate function that provides a shared runtime): +lazy_static! { + static ref SHARED_RUNTIME: Runtime = Runtime::new().unwrap(); +} // Then in your functions: - let result = tokio::runtime::Runtime::new().unwrap().block_on(async { + let result = SHARED_RUNTIME.block_on(async { // ... });Also applies to: 151-151, 263-263, 334-334
47-49: Improve error detail on GCS store creation failure.When an error occurs while creating the GCS store (line 47), the code logs a debug message and returns
None. Consider including a more detailed error message or returning a specific error value through the FFI boundary to facilitate better troubleshooting and user feedback.
178-183: Differentiate document not found from other errors for better debugging.Both a missing document (lines 195-198) and a load failure (lines 199-202) ultimately return no result. This results in the same
null_mut()outcome in the FFI. Differentiating these scenarios with distinct error codes or messages can help upstream callers handle “not found” vs. “load failure” more accurately.Also applies to: 195-202
224-293: Refactor repeated config and pool creation logic to a helper function.Both
get_document_history(lines 224-293) androllback_document(lines 295-363) duplicate the same steps for parsing theConfigand creating the store pool. Consolidate these steps into a shared helper to reduce boilerplate and improve maintainability.-fn get_document_history(...){ - // parse doc_id - // parse config - // create pool - // ... -} - -fn rollback_document(...){ - // parse doc_id - // parse config - // create pool - // ... -} +fn parse_config_and_create_pool(config_json: *const c_char) -> Option<(String, Arc<BroadcastPool>)> { + // ... +} // Inside each FFI function, just call parse_config_and_create_pool to reuse logicAlso applies to: 295-363
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
server/api/.env.example(1 hunks)server/api/internal/app/config/config.go(2 hunks)server/api/internal/app/repo.go(2 hunks)server/api/internal/usecase/websocket/document.go(1 hunks)server/websocket/src/bin/websocket.rs(1 hunks)server/websocket/src/broadcast/pool.rs(1 hunks)server/websocket/src/ffi.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- server/api/.env.example
- server/api/internal/app/config/config.go
- server/api/internal/usecase/websocket/document.go
- server/api/internal/app/repo.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci-api / ci-api-test
🔇 Additional comments (9)
server/websocket/src/broadcast/pool.rs (2)
49-95: Avoid potential concurrency race condition inget_or_create_group.Multiple concurrent calls for the same
doc_idmay lead to creating separateBroadcastGroupinstances before one is inserted, especially because this block containsawaitcalls after checking for a vacant entry. A double-checked locking pattern or use of DashMap’sentryAPI with atomic insertion can ensure that only one group is generated perdoc_id.
113-126: Sync cleanup logic to avoid race conditions with freshly connected clients.The deferred cleanup triggered by
remove_connectioncan still collide with connections that arrive in the one-second interval. Consider using a synchronization mechanism or a more robust approach to guard this gap in time.server/websocket/src/bin/websocket.rs (4)
1-10: LGTM! Well-organized imports with proper feature gating.The imports are cleanly organized and the conditional compilation for authentication is properly implemented.
20-26: LGTM! Proper error handling for configuration loading.The configuration loading includes appropriate error handling with logging and graceful exit.
36-40: Remove or implement the commented bucket existence check.The commented code for bucket existence check should either be removed if not needed or implemented properly.
Please clarify if this check is still required. If it is, I can help implement it with proper retry logic as suggested in the previous review.
49-66: LGTM! Clean conditional state initialization with proper error handling.The application state initialization is well-structured with appropriate feature flags and error handling for the auth service.
server/websocket/src/ffi.rs (3)
13-29: Initialization strategy is solid.Your use of a
Onceguard to ensure tracing is only initialized once is a good pattern. This approach helps avoid duplicate logging setups across multiple invocations.
303-303: Validate negative or excessively large rollback clock.You are casting
target_clock(a signed integer) tou32. A negativetarget_clockwould wrap unexpectedly, possibly rolling back to a huge clock number. Consider verifyingtarget_clock >= 0to avoid unintended rollbacks or potential overflow if provided as a very large positive integer.Below is a sample snippet to check the range:
- .rollback_to(&doc_id, target_clock as u32) + if target_clock < 0 { + debug!("FFI: Negative target_clock is invalid"); + return std::ptr::null_mut(); + } + let target_clock_u32 = target_clock as u32; + .rollback_to(&doc_id, target_clock_u32)
365-376: Properly finalizing memory.The
free_stringfunction correctly frees the string pointer returned by FFI calls. This ensures no memory leak occurs when the caller invokesfree_stringon the string.
| // Create broadcast pool | ||
| let pool = Arc::new(BroadcastPool::new(store, Some(config.redis))); | ||
| tracing::info!("Broadcast pool initialized"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for Redis connection initialization.
The broadcast pool initialization should handle potential Redis connection failures.
- let pool = Arc::new(BroadcastPool::new(store, Some(config.redis)));
+ let pool = match BroadcastPool::try_new(store, Some(config.redis)).await {
+ Ok(pool) => Arc::new(pool),
+ Err(e) => {
+ error!("Failed to initialize broadcast pool: {}", e);
+ std::process::exit(1);
+ }
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Create broadcast pool | |
| let pool = Arc::new(BroadcastPool::new(store, Some(config.redis))); | |
| tracing::info!("Broadcast pool initialized"); | |
| // Create broadcast pool | |
| let pool = match BroadcastPool::try_new(store, Some(config.redis)).await { | |
| Ok(pool) => Arc::new(pool), | |
| Err(e) => { | |
| error!("Failed to initialize broadcast pool: {}", e); | |
| std::process::exit(1); | |
| } | |
| }; | |
| tracing::info!("Broadcast pool initialized"); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/websocket/src/conf.rs (1)
58-62: Consider failing in production if.envis missing.
Warning about a missing.envfile is acceptable for development, but you may want to fail hard in production to avoid uninitialized critical variables.if dotenv::from_path(Path::new(".env")).is_ok() { info!("Loaded configuration from .env file"); } else { - warn!("No .env file found, using environment variables"); + if env::var("REEARTH_FLOW_APP_ENV").unwrap_or_default() == "production" { + return Err(ConfigError::MissingRequired(".env file in production".to_string())); + } else { + warn!("No .env file found, using environment variables"); + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/websocket/.env.docker(1 hunks)server/websocket/src/conf.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/websocket/.env.docker
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci-api / ci-api-test
🔇 Additional comments (5)
server/websocket/src/conf.rs (5)
10-20: Remove default values for sensitive configurations.
Storing default values for credentials or other sensitive fields can inadvertently lead to production misconfigurations. Rely on environment variables or explicit dev-only defaults.Apply this diff to remove default values and prompt developers to supply these explicitly:
-const DEFAULT_REDIS_URL: &str = "redis://127.0.0.1:6379"; -const DEFAULT_GCS_BUCKET: &str = "yrs-dev"; -const DEFAULT_GCS_ENDPOINT: &str = "http://localhost:4443"; -#[cfg(feature = "auth")] -const DEFAULT_AUTH_URL: &str = "http://localhost:8080"; -const DEFAULT_ORIGINS: &str = "http://localhost:3000"; -const DEFAULT_WS_PORT: &str = "8000"; +// Remove or limit sensitive defaults to development environment only. // For production, error out if not provided via env variables.
21-27: Enhance error handling with validation errors.
Introduce validations for missing or invalid fields by adding dedicated errors likeMissingRequiredorInvalidEnvironmentto theConfigErrorenum so that the application fails fast on critical misconfigurations.#[derive(Debug, Error)] pub enum ConfigError { #[error("Environment variable error: {0}")] EnvVar(#[from] env::VarError), #[error("Invalid value for {key}: {value}")] InvalidValue { key: String, value: String }, + #[error("Missing required configuration: {0}")] + MissingRequired(String), + #[error("Invalid configuration for {env}: {message}")] + InvalidEnvironment { env: String, message: String }, }
29-48: Avoid exposing secrets via#[derive(Debug)].
AuthConfigandConfigcan contain sensitive URLs or configuration details. DerivingDebugcould leak these values in logs or error messages. Remove or mask sensitive fields in debug output.-#[derive(Debug, Deserialize, Clone)] +#[derive(Deserialize, Clone)] pub struct AuthConfig { pub url: String, } -#[derive(Debug, Deserialize, Clone)] +#[derive(Deserialize, Clone)] pub struct Config { // ... }
103-109: Redact sensitive configuration logs.
Logging entire config structs exposes internal details (URLs, secrets, etc.). Redact or minimize sensitive values to mitigate leaks.info!("Final configuration:"); -info!("Redis: {:?}", config.redis); -info!("GCS: {:?}", config.gcs); -info!("App: {:?}", config.app); +info!("Redis: <redacted>"); +info!("GCS bucket: {}", config.gcs.bucket_name); +info!("App environment: {}", config.app.env); #[cfg(feature = "auth")] -info!("Auth: {:?}", config.auth); +info!("Auth configured: true");
184-215: ReturnResultfrombuildto handle invalid states.
If critical fields are missing or invalid, the code should fail early rather than silently applying defaults.-pub fn build(self) -> Config { +pub fn build(self) -> Result<Config, ConfigError> { // Example: check required fields if self.redis_url.is_none() { return Err(ConfigError::MissingRequired("redis_url".to_string())); } let config = Config { redis: RedisConfig { url: self .redis_url .unwrap_or_else(|| DEFAULT_REDIS_URL.to_string()), ttl: self.redis_ttl.unwrap_or(DEFAULT_REDIS_TTL), }, // ... rest of the fields ... }; - config + Ok(config) }
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
server/websocket/src/bin/websocket.rs (3)
37-41: 🛠️ Refactor suggestionImplement retry logic for bucket existence check.
The current implementation could benefit from retry logic for bucket existence check.
46-48: 🛠️ Refactor suggestionAdd error handling for Redis connection initialization.
The broadcast pool initialization should handle potential Redis connection failures.
69-73: 🛠️ Refactor suggestionImplement graceful shutdown handling.
The server startup should handle graceful shutdown and provide more detailed error context.
🧹 Nitpick comments (2)
server/websocket/src/bin/websocket.rs (2)
14-19: Consider enhancing tracing setup with spans and request tracking.While the basic tracing setup is good, consider adding:
- Span information for request tracking
- Dynamic log level configuration from environment
tracing_subscriber::fmt() - .with_max_level(tracing::Level::INFO) + .with_env_filter(tracing_subscriber::EnvFilter::from_default_env() + .add_directive(tracing::Level::INFO.into())) .with_file(true) .with_line_number(true) + .with_span_events(tracing_subscriber::fmt::format::FmtSpan::FULL) .init();
21-27: Enhance error context for configuration failures.While the error handling is appropriate, consider providing more context about which configuration values failed to load.
let config = match Config::load() { Ok(config) => config, Err(e) => { - error!("Failed to load config: {}", e); + error!("Failed to load config: {:#}. Please check your environment variables and configuration files.", e); std::process::exit(1); } };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/websocket/.env.docker(1 hunks)server/websocket/src/bin/websocket.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/websocket/.env.docker
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci-api / ci-api-test
🔇 Additional comments (2)
server/websocket/src/bin/websocket.rs (2)
1-11: LGTM! Well-organized imports with proper feature flag usage.The imports are appropriately scoped and the conditional compilation for auth features follows Rust best practices.
50-67: LGTM! Robust application state initialization.The implementation properly handles:
- Conditional auth service initialization
- Error handling for auth service
- Feature flag usage for conditional compilation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/readme.adoc (2)
5-8: Admonition Block Formatting
The use of the[IMPORTANT]tag followed by the====block effectively highlights a prerequisite step. Ensure that this styling remains consistent with other parts of your documentation for a uniform look and feel.
24-31: Concise WebSocket Server Startup Instruction & Framework Note
The instructions for starting the WebSocket server are clear and precise. Given the PR objective of updating the websocket server to use the yrs-warp framework, consider adding a brief note or section that highlights any framework-specific changes or prerequisites (e.g., environment variables, configuration differences, etc.) to better inform users of the migration impact.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/readme.adoc(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci-api / ci-api-test
🔇 Additional comments (4)
server/readme.adoc (4)
1-4: Clear Section Headers and Hierarchy
The "Getting Started" and "Build and Run Instructions" headers are well-structured and clearly indicate the start of the documentation. This helps new users easily locate the setup instructions.
9-13: Well-Formatted Code Block for Building
The code block delineated by[source,bash]and the----markers clearly conveys the build command (cargo build --release). This clear separation of command and explanation aids user comprehension.
15-17: Service Segmentation Clarity
The "Running the Services" header and the subsequent instruction to start the API server are clearly defined. This separation ensures users understand the sequential steps required for the server setup.
18-23: Accurate API Server Startup Command
The added code block showinggo run ./cmd/reearth-flow/is straightforward and easy to follow. It clearly specifies the command to initiate the API server, which is crucial since the API server provides the authentication service required by the WebSocket server.
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
New Features
AuthServicefor API token verification with enhanced error handling.Infrastructure Enhancements
Developer Experience