refactor(websocket): update websocket server to yrs-warp #786
Conversation
WalkthroughThis pull request removes a large number of legacy and unused files from the WebSocket project. Configuration files (.dockerignore, .taplo.toml, Makefile.toml, various Dockerfiles) have been deleted or simplified. The Cargo.toml has been completely restructured for the websocket package with updated metadata and dependencies. In addition, most of the app, handler, and state management modules (in app, crates/infra, and crates/services) have been removed. New endpoints and UI authentication logic have been added in the API and UI layers, and the server’s websocket implementation has been overhauled with new modules for authentication, broadcast, connection, and storage. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UI
participant AuthService
participant Server
Client->>UI: Request WebSocket connection
UI->>AuthService: Retrieve access token via useAuth
AuthService-->>UI: Return access token
UI->>Server: Initiate WS upgrade request with token
Server->>AuthService: Verify token (POST /verify/ws-token)
AuthService-->>Server: Authorized response ({"authorized": true})
Server->>Client: Establish WebSocket connection
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
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: team@example.com +: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.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #786 +/- ##
==========================================
+ Coverage 25.56% 25.63% +0.06%
==========================================
Files 138 138
Lines 8073 8173 +100
==========================================
+ Hits 2064 2095 +31
- Misses 5806 5870 +64
- Partials 203 208 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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 <b.sypytkowski@gmail.com>"] -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 <b.sypytkowski@gmail.com>"] -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 <b.sypytkowski@gmail.com>"] -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
websocket/docker-compose.yml
Outdated
| volumes: | ||
| # Define persistent volume for Redis data | ||
| redis_data: | ||
| gcs-data: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define all required volumes for data persistence.
Add the Redis volume definition to ensure data persistence for both services.
volumes:
gcs-data:
+ redis-data:📝 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.
| volumes: | |
| # Define persistent volume for Redis data | |
| redis_data: | |
| gcs-data: | |
| volumes: | |
| gcs-data: | |
| redis-data: |
…n, and refactor config handling
…erifyAuth endpoint
…ter and adjust AuthService structure
…remove YAML config, and adjust Redis and GCS settings
…Dockerfile to copy it into the container
…et authentication flow; remove unused .env file
…configuration URLs in .env and Rust files
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.
websocket/benches/server_bench.rs
Outdated
| // Reduce timeout duration | ||
| const TIMEOUT_DURATION: Duration = Duration::from_millis(50); | ||
| // Shorter delays for connection operations | ||
| const CONNECTION_DELAY: Duration = Duration::from_millis(10); | ||
| const CLOSE_DELAY: Duration = Duration::from_millis(20); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider increasing timeout durations for more reliable operation.
The current timeout durations are quite aggressive and might lead to flaky tests:
TIMEOUT_DURATION: 50ms might be too short for message verificationCONNECTION_DELAY: 10ms might not be enough for connection establishment
Consider increasing these values based on your network latency. You can measure the actual latency using:
🏁 Script executed:
#!/bin/bash
# Measure WebSocket latency
time curl -N -H "Connection: Upgrade" \
-H "Upgrade: websocket" \
-H "Host: localhost:8080" \
-H "Origin: http://localhost:8080" \
http://localhost:8080/bench-docLength of output: 519
Increase Timeout Durations in WebSocket Benchmarks
The latency measurement (~217 ms) indicates that the current aggressive timeout values (50 ms for TIMEOUT_DURATION, 10 ms for CONNECTION_DELAY, and 20 ms for CLOSE_DELAY) could lead to unreliable or flaky benchmarking. Although the curl command returned "Not Found" (suggesting the endpoint may not be properly configured), the elapsed time clearly shows that real network delays exceed these very short intervals.
- Verify that the WebSocket endpoint is correctly set up to expect upgrade requests.
- Consider increasing
TIMEOUT_DURATIONto at least 200 ms to better accommodate typical network delays. - Adjust
CONNECTION_DELAYandCLOSE_DELAYaccordingly to ensure stable connection operations.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ui/src/lib/yjs/useYjsSetup.ts (1)
36-62:⚠️ Potential issueAdd error handling for token retrieval and WebSocket connection.
The implementation needs error handling for both token retrieval and WebSocket connection failures.
Apply this diff to add comprehensive error handling:
(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, + }, + }, + ); + + yWebSocketProvider.on('connection-error', (error: Error) => { + console.error('WebSocket connection error:', error); + }); + + yWebSocketProvider.on('connection-close', () => { + console.warn('WebSocket connection closed'); + }); yWebSocketProvider.once("sync", () => { // ... existing sync handler code ... }); + } catch (error) { + console.error('Setup failed:', error); + } })();
🧹 Nitpick comments (1)
ui/src/lib/yjs/useYjsSetup.ts (1)
6-6: Fix import order.Reorder imports to follow project conventions by moving
@flow/lib/authafter@flow/global-constants.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";🧰 Tools
🪛 GitHub Check: ci-ui / ci
[warning] 6-6:
@flow/lib/authimport should occur after import of@flow/global-constants
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
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
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Collect information
- GitHub Check: Redirect rules - reearth-flow
- GitHub Check: Header rules - reearth-flow
- GitHub Check: Pages changed - reearth-flow
🔇 Additional comments (2)
ui/src/lib/yjs/useYjsSetup.ts (2)
17-17: LGTM!The
useAuthhook is correctly destructured to obtain thegetAccessTokenfunction.
76-76: LGTM!The
getAccessTokenfunction is correctly added to the dependency array to prevent stale closure issues.
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 (2)
websocket/src/bin/main.rs(1 hunks)websocket/src/conf.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Collect information
🔇 Additional comments (7)
websocket/src/conf.rs (2)
112-118: Avoid logging sensitive configuration valuesLogging the entire configuration may expose sensitive information such as URLs and origins. To prevent potential security risks, consider masking sensitive fields or logging them at a debug level.
56-120: Add configuration validation to ensure critical configuration values are validAdding a validation method to the
Configstruct can help catch misconfigurations early. This method should verify that essential configuration values are present and meet expected criteria, providing informative error messages if validation fails.websocket/src/bin/main.rs (5)
33-34: Move hardcoded configuration values to the configuration systemThe constants
BUCKET_NAMEandPORTare hardcoded, limiting flexibility and maintainability. To enhance configurability, consider moving these values into theConfigstruct and loading them from environment variables or a configuration file.
44-47: Move bucket location to configurationThe bucket location is hardcoded as
"US", which might not suit all deployment regions. To support different regions and enhance flexibility, consider making the bucket location configurable via theConfigstruct.
54-58: Improve error handling by matching specific error codesRelying on error message strings to determine if a bucket already exists is fragile and may break if the message changes. Instead, use specific error codes or patterns provided by the GCS client library to accurately detect the "bucket already exists" condition.
92-95: Avoid logging sensitive information such as authentication tokensLogging the authentication token in warning messages can expose sensitive information. To enhance security, avoid logging tokens or any sensitive data.
Apply this diff to remove the token from the log message:
tracing::warn!( - "Authentication failed for doc_id: {}, token: {}", - doc_id, - auth.token + "Authentication failed for doc_id: {}", + doc_id );
178-181: Handle potential binding errors when starting the serverUsing
unwrap()on thebindandservemethods may cause the application to panic if binding to the address fails. To improve robustness, handle these errors gracefully and provide informative messages.Apply this diff to handle binding and serving errors:
tracing::info!("Starting server on 0.0.0.0:{}", PORT); -let listener = tokio::net::TcpListener::bind(format!("0.0.0.0:{}", PORT)) - .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 { +if let Err(e) = axum::serve(listener, app).await { tracing::error!("Server error: {}", e); }
There was a problem hiding this comment.
Actionable comments posted: 24
🧹 Nitpick comments (10)
server/websocket/benches/server_bench.rs (4)
9-13: Consider making timeout and delay durations configurableThe hard-coded constants for
TIMEOUT_DURATION,CONNECTION_DELAY, andCLOSE_DELAYmay not be optimal in all environments. Making these durations configurable could enhance the flexibility and reliability of the benchmarks, accommodating variations in system performance.
44-50: Parameterize the WebSocket server addressThe
connect_clientfunction uses a hardcoded server addressws://127.0.0.1:8080/. To enhance flexibility and accommodate different testing environments, consider parameterizing the server address or reading it from a configuration file or environment variable.
79-93: Refactor to reuse the runtime instance across benchmarksEach benchmark function creates its own Tokio runtime instance. This introduces unnecessary overhead and code duplication. Consider initializing the runtime once and reusing it across all benchmark functions to improve efficiency and reduce redundancy.
Apply this diff to create a shared runtime:
+// Initialize the runtime outside of benchmark functions +lazy_static::lazy_static! { + static ref RT: 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 { + RT.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; }); }) }); }Repeat this change in other benchmark functions to use the shared runtime.
98-134: Extract common code into helper functions to reduce duplicationThe benchmark functions
bench_doc_sync,bench_text_operations, andbench_large_updatecontain duplicated code for setting up connections, creating documents, performing operations, sending updates, verifying message reception, and closing connections. Refactoring this common code into reusable helper functions will improve maintainability and reduce code repetition.Also applies to: 140-180, 320-354
server/websocket/src/storage/sqlite/mod.rs (1)
56-67: Simplify error handling in thegetmethodThe current implementation of the
getmethod checks for the presence of a row and then retrieves the value. This can be simplified using thequery_rowmethod, which returns anOptionalresult and can streamline the code.Apply this diff to simplify the
getmethod:- let mut stmt = conn.prepare("SELECT value FROM kv_store WHERE key = ?")?; - let mut rows = stmt.query(params![key])?; - - if let Some(row) = rows.next()? { - Ok(Some(row.get(0)?)) - } else { - Ok(None) - } + let result = conn.query_row( + "SELECT value FROM kv_store WHERE key = ?", + params![key], + |row| row.get(0), + ).optional()?; + Ok(result)server/websocket/src/auth/mod.rs (1)
29-47: Consider adding retry logic for token verification.The token verification is crucial for security but lacks retry logic for transient network issues. Also, consider adding rate limiting to prevent potential DoS attacks.
pub async fn verify_token(&self, token: &str) -> Result<bool> { { + let retry_policy = ExponentialBackoff::builder() + .with_max_retries(3) + .build(); + let response = self .client .post(&self.config.url) .header("Authorization", format!("Bearer {}", token)) + .middleware(RetryMiddleware::new(retry_policy)) .send() .await?; tracing::debug!("response: {:?}", response);server/websocket/src/storage/kv/keys.rs (1)
153-158: Remove commented-out code.The commented-out implementation for
ToMdbValueshould be either implemented or removed to maintain clean code.server/websocket/Dockerfile (1)
6-6: Optimize build cache usage.Copy only necessary files for dependency resolution first to optimize build cache usage.
WORKDIR /usr/src/app +COPY Cargo.toml Cargo.lock ./ +RUN mkdir src && touch src/lib.rs +RUN cargo fetch COPY . . RUN cargo build --releaseserver/websocket/README.adoc (1)
33-42: Running the Server: Execution Instructions
The running instructions (invokingsh build.shfollowed bycd outputandsh run.sh) provide a straightforward path to start the server. It might be beneficial to remind users to verify that these scripts have executable permissions (e.g., viachmod +x).server/websocket/Cargo.toml (1)
52-59: Dev-Dependencies: Version Consistency Check
The development dependencies are clearly outlined. Note thattokioappears in both the primary and dev sections with slightly different version pinning (e.g.,"1.43"vs."1"). It may be worthwhile to align these version requirements for consistency, if possible.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
server/websocket/Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (26)
server/websocket/.env-sample(1 hunks)server/websocket/.gitignore(1 hunks)server/websocket/Cargo.toml(1 hunks)server/websocket/Dockerfile(1 hunks)server/websocket/LICENSE(1 hunks)server/websocket/README.adoc(1 hunks)server/websocket/benches/readme.adoc(1 hunks)server/websocket/benches/server_bench.rs(1 hunks)server/websocket/build.sh(1 hunks)server/websocket/docker-compose.yml(1 hunks)server/websocket/script/run.sh(1 hunks)server/websocket/src/auth/mod.rs(1 hunks)server/websocket/src/bin/main.rs(1 hunks)server/websocket/src/broadcast/group.rs(1 hunks)server/websocket/src/broadcast/mod.rs(1 hunks)server/websocket/src/broadcast/pool.rs(1 hunks)server/websocket/src/conf.rs(1 hunks)server/websocket/src/conn.rs(1 hunks)server/websocket/src/lib.rs(1 hunks)server/websocket/src/storage/gcs/mod.rs(1 hunks)server/websocket/src/storage/kv/error.rs(1 hunks)server/websocket/src/storage/kv/keys.rs(1 hunks)server/websocket/src/storage/kv/mod.rs(1 hunks)server/websocket/src/storage/mod.rs(1 hunks)server/websocket/src/storage/sqlite/mod.rs(1 hunks)server/websocket/src/ws.rs(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- server/websocket/src/broadcast/mod.rs
- server/websocket/src/storage/kv/error.rs
- server/websocket/.gitignore
- server/websocket/LICENSE
- server/websocket/src/storage/mod.rs
- server/websocket/.env-sample
🧰 Additional context used
🪛 Shellcheck (0.10.0)
server/websocket/build.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
server/websocket/script/run.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🔇 Additional comments (20)
server/websocket/src/ws.rs (1)
14-22: Well-structuredWarpConnimplementationThe
WarpConnstruct and itsnewmethod are correctly implemented, providing a clean wrapper over theConnectionwith proper initializations.server/websocket/src/conn.rs (1)
31-45: Verify the necessity of theasynckeyword inwith_broadcast_groupThe
with_broadcast_groupmethod is declared asasync, but there are noawaitexpressions within its body unlessbroadcast_group.subscribeis asynchronous. Ifsubscribeis a synchronous function, consider removing theasynckeyword to simplify the code.server/websocket/src/conf.rs (1)
56-120: Configuration loading logic looks goodThe configuration loading mechanism is well-structured, with appropriate default values and error handling for missing or invalid environment variables.
server/websocket/src/broadcast/group.rs (1)
60-72: Connection management methods look goodThe methods for incrementing, decrementing, and retrieving connection counts are implemented correctly and follow best practices.
server/websocket/src/lib.rs (1)
1-16: Module setup and type alias look goodThe module declarations and type alias
AwarenessRefare well-defined and follow Rust best practices.server/websocket/docker-compose.yml (4)
1-13: WebSocket Service Configuration: Clear and Integrated
The configuration for the WebSocket service is clear. It correctly builds from the local Dockerfile, exposes port 8000, loads environment variables from a designated file, and sets up dependencies on Redis and Fake-GCS.
14-18: Redis Service Setup: Version & Port Mapping
The Redis service uses the officialredis:7-alpineimage with an appropriate port mapping (6379:6379). This setup adheres to best practices; verify that the Redis configuration meets production requirements if needed.
19-26: Fake-GCS Service Configuration: Command & Volumes
The Fake-GCS service is configured with thefsouza/fake-gcs-serverimage, with ports and command (["-scheme", "http"]) correctly specified. The volume mapping for persisting data is also properly set up.
27-29: Volumes Definition: Persistent Storage Setup
The volumegcs-datais declared correctly for persistent storage of the Fake-GCS service.server/websocket/README.adoc (4)
1-7: Project Title & Introduction: Clear Overview
The title and introductory section provide a succinct overview of the WebSocket server’s purpose. This sets a good tone for the documentation.
8-15: Features Section: Informative and Concise
The features list covers the key aspects of the server such as WebSocket implementation, Y-Protocol support, broadcasting, local storage, and handling multi-client connections.
16-32: Quick Start - Server Installation: Step-by-Step Clarity
The installation instructions (including the Docker run command for Fake-GCS and Cargo build steps) are clear. Ensure that the commands (e.g. the Docker run command with-scheme http) remain consistent with the settings in the docker-compose file.
44-57: Project Structure: Organized and Intuitive
The project structure section clearly outlines the directory layout and key modules. This helps developers quickly locate components of interest.server/websocket/Cargo.toml (4)
1-12: Package Metadata: Well-Defined and Informative
The package metadata is comprehensive—with name, version, edition, description, license, authors, and repository information appropriately defined.
13-16: Feature Flags: Concise and Extensible
The features section sets a default feature (auth) and leaves room for extensibility. Ensure that documentation on feature usage is maintained as the project evolves.
17-51: Dependencies: Explicit Versioning and Feature Usage
A comprehensive list of dependencies is specified with explicit versions and enabled features.
- Consider reviewing the commented out dependency (
# lmdb-rs) and removing it if it is no longer required.- Overall, the dependency list is well-organized and aligns with the new architecture.
60-63: Benchmark Configuration: Explicit and Functional
The benchmark section is correctly added with[[bench]]and specifies that harnessing is disabled. This is useful for custom benchmark setups.server/websocket/benches/readme.adoc (3)
1-12: Benchmark Suite Overview: Comprehensive Introduction
The initial section introduces the benchmark suite effectively, outlining its purpose and scope. The table of contents management is also neatly handled.
23-73: Benchmark Descriptions: Detailed and Clear
The descriptions for various benchmark tests (connection tests, document operations, and broadcasting) are well-articulated. They provide specific details on what each test measures and how performance is evaluated.
75-95: Configuration & Interpreting Results: Informative Guidance
The sections on configuration and interpreting results offer useful insights into default settings and key performance metrics. This helps users understand how to analyze benchmark outcomes.
| { | ||
| let mut txn = doc.transact_mut(); | ||
| match self.store.load_doc(doc_id, &mut txn).await { | ||
| Ok(_) => { | ||
| tracing::debug!("Successfully loaded existing document: {}", doc_id); | ||
| } | ||
| Err(e) => { | ||
| tracing::warn!( | ||
| "No existing document found or failed to load {}: {}", | ||
| doc_id, | ||
| e | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle document load errors explicitly
When loading the document state in get_or_create_group, errors are logged but not handled beyond that. If loading fails due to reasons other than the document not existing, this could lead to inconsistent states. Consider returning an error or handling specific error cases to ensure robustness.
| pub fn doc_meta_name(key: &[u8]) -> &[u8] { | ||
| &key[7..(key.len() - 1)] | ||
| } | ||
|
|
||
| pub fn doc_oid_name(key: &[u8]) -> &[u8] { | ||
| &key[2..(key.len() - 1)] | ||
| } |
There was a problem hiding this comment.
Add bounds checking for key slicing operations.
The slice operations in doc_meta_name and doc_oid_name could panic if the key is too short. Consider adding bounds checking.
pub fn doc_meta_name(key: &[u8]) -> &[u8] {
+ if key.len() < 8 {
+ return &[];
+ }
&key[7..(key.len() - 1)]
}
pub fn doc_oid_name(key: &[u8]) -> &[u8] {
+ if key.len() < 3 {
+ return &[];
+ }
&key[2..(key.len() - 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.
| pub fn doc_meta_name(key: &[u8]) -> &[u8] { | |
| &key[7..(key.len() - 1)] | |
| } | |
| pub fn doc_oid_name(key: &[u8]) -> &[u8] { | |
| &key[2..(key.len() - 1)] | |
| } | |
| pub fn doc_meta_name(key: &[u8]) -> &[u8] { | |
| if key.len() < 8 { | |
| return &[]; | |
| } | |
| &key[7..(key.len() - 1)] | |
| } | |
| pub fn doc_oid_name(key: &[u8]) -> &[u8] { | |
| if key.len() < 3 { | |
| return &[]; | |
| } | |
| &key[2..(key.len() - 1)] | |
| } |
| 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/* |
There was a problem hiding this comment.
Improve script robustness and error handling.
The script needs several improvements for better reliability and error handling.
+#!/bin/bash
+set -euo pipefail
+
+# Create output directory if it doesn't exist
mkdir -p output
+
+# Copy configuration files
cp script/* output 2>/dev/null || true
cp -r src/conf.yaml output/ 2>/dev/null || true
+
+# Build and copy binary
cargo build --bin main --release
+if [ $? -ne 0 ]; then
+ echo "Build failed"
+ exit 1
+fi
+
cp target/release/main output
chmod +x output/*📝 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.
| 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/* | |
| #!/bin/bash | |
| set -euo pipefail | |
| # Create output directory if it doesn't exist | |
| mkdir -p output | |
| # Copy configuration files | |
| cp script/* output 2>/dev/null || true | |
| cp -r src/conf.yaml output/ 2>/dev/null || true | |
| # Build and copy binary | |
| cargo build --bin main --release | |
| if [ $? -ne 0 ]; then | |
| echo "Build failed" | |
| 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)
| COPY --from=builder /usr/src/app/target/release/main /usr/local/bin/websocket | ||
|
|
||
| WORKDIR /usr/local/bin | ||
| COPY .env . |
There was a problem hiding this comment.
Avoid copying .env file into container.
Copying the .env file into the container is not a secure practice. Consider using environment variables or secrets management.
Replace the .env file with environment variables passed at runtime:
-COPY .env .
+# Use environment variables passed at runtime
+# Example: docker run -e REDIS_URL=redis://redis:6379 ...📝 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.
| COPY .env . | |
| # Use environment variables passed at runtime | |
| # Example: docker run -e REDIS_URL=redis://redis:6379 ... |
| * Detailed timing information | ||
| * Historical performance data | ||
|
|
||
| Key metrics to look for: |
There was a problem hiding this comment.
Critical Note: Server Port Mismatch
The notes section instructs users to ensure the WebSocket server is running on localhost:8080 before running benchmarks, but the docker-compose file exposes the service on port 8000. This inconsistency should be resolved to avoid confusion.
| @@ -0,0 +1 @@ | |||
| ./main | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Shebang Missing in Script
The run script currently only contains ./main. To ensure consistent behavior across environments and to satisfy shellcheck recommendations, please add a shebang (e.g., #!/usr/bin/env bash) at the top of the file.
Proposed Diff:
-./main
+#!/usr/bin/env bash
+
+./main📝 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.
| ./main | |
| #!/usr/bin/env bash | |
| ./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 (8)
server/websocket/src/ws.rs (2)
11-12: Enhance documentation with protocol details.The current documentation could be more informative. Consider adding:
- Protocol version/specification details
- Expected message format
- Connection lifecycle
- Error handling behavior
47-81: Consider implementing custom error conversion.Instead of using
Error::Other(e.into())for all error cases, consider implementing specific error conversions to provide more detailed error context.Example implementation:
impl From<axum::Error> for Error { fn from(e: axum::Error) -> Self { match e { // Map specific axum errors to yrs errors axum::Error::ConnectionClosed => Error::ConnectionClosed, _ => Error::Other(e.into()), } } }server/websocket/src/storage/kv/keys.rs (1)
56-58: Remove redundant function.The
key_doc_startfunction is redundant as it's just an alias forkey_doc. Consider removing it or documenting why it exists.-pub fn key_doc_start(oid: OID) -> Result<Key<8>, std::io::Error> { - key_doc(oid) -}server/websocket/src/bin/main.rs (5)
25-25: Remove commented-out code.Remove the commented-out import as it's no longer needed and can cause confusion.
-//use websocket::storage::sqlite::SqliteStore;
34-34: Make port configurable through environment variables.Hardcoding the port number limits flexibility. Consider making it configurable through environment variables with a default fallback.
-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()) +}
38-41: Add validation for empty authentication tokens.The current implementation allows empty tokens due to
#[serde(default)]. Consider adding validation to reject empty tokens early.#[derive(Debug, Deserialize)] struct AuthQuery { #[serde(default)] token: String, + + #[inline] + fn is_valid(&self) -> bool { + !self.token.trim().is_empty() + } }
44-47: Make bucket location configurable.The bucket location is hardcoded to "US". Consider making it configurable through the Config struct.
- let bucket = BucketCreationConfig { - location: "US".to_string(), - ..Default::default() - }; + let bucket = BucketCreationConfig { + location: config.gcs.location.unwrap_or_else(|| "US".to_string()), + ..Default::default() + };
118-130: Add support for graceful shutdown.The current implementation doesn't handle graceful shutdown of WebSocket connections. Consider adding shutdown signal handling.
async fn handle_socket( socket: WebSocket, bcast: Arc<BroadcastGroup>, doc_id: String, pool: Arc<BroadcastPool>, + mut shutdown: tokio::sync::broadcast::Receiver<()>, ) { bcast.increment_connections(); let conn = WarpConn::new(bcast, socket); - if let Err(e) = conn.await { + tokio::select! { + result = conn => if let Err(e) = result { + tracing::error!("WebSocket connection error: {}", e); + }, + _ = shutdown.recv() => { + tracing::info!("Shutting down WebSocket connection for doc_id: {}", doc_id); + } + } pool.remove_connection(&doc_id).await; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
server/websocket/src/bin/main.rs(1 hunks)server/websocket/src/broadcast/pool.rs(1 hunks)server/websocket/src/storage/kv/keys.rs(1 hunks)server/websocket/src/ws.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/websocket/src/broadcast/pool.rs
🔇 Additional comments (8)
server/websocket/src/ws.rs (2)
13-30: LGTM! Clean implementation with zero-cost abstraction.The WarpConn implementation is well-structured:
- Uses #[repr(transparent)] appropriately
- Implements Future trait correctly
- Proper error handling in place
98-118: LGTM! Message handling is complete and correct.The Stream implementation properly handles all message types, including the correct handling of Message::Close(_) as suggested in the previous review.
server/websocket/src/storage/kv/keys.rs (3)
1-40: LGTM! Well-documented key patterns and constants.The constants are well-organized and the documentation clearly explains the key patterns and their structure.
159-164: Address commented-out code.The commented-out
ToMdbValueimplementation suggests incomplete integration. Either:
- Remove the commented code if it's no longer needed
- Implement it if it's required
- Add a TODO comment explaining why it's kept
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for MdbValue usage in the codebase. # Search for MdbValue type and implementations rg 'MdbValue|ToMdbValue' # Search for related database integration code rg 'mdb|lmdb'Length of output: 694
Action Required: Clarify the Status of the Unused ToMdbValue Implementation
It appears that the only occurrence of any MdbValue integration is the commented-out ToMdbValue implementation in server/websocket/src/storage/kv/keys.rs, and there are no active references to MdbValue elsewhere in the codebase. Please consider the following options:
- Remove the commented-out code if MdbValue integration isn’t planned or needed.
- Implement the functionality if MdbValue conversion is required.
- Alternatively, add a clear TODO comment explaining why the code remains commented out.
97-104: Consider increasing SmallVec size for metadata keys.The fixed size of 20 bytes for metadata keys might be too small for some metadata names. Consider increasing the size or using a larger SmallVec to accommodate longer metadata names.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for metadata key lengths in the codebase. # Search for metadata key usage patterns rg -A 5 'key_meta\(' # Search for metadata-related constants or configurations rg -A 5 'META|[Mm]etadata'Length of output: 80163
Metadata key inline capacity appears sufficient based on current usage
After reviewing the implementation of key_meta in server/websocket/src/storage/kv/keys.rs and how it’s used throughout the codebase, we see that the metadata key is constructed by concatenating a few fixed bytes (version, keyspace, OID conversion, SUB_META, and TERMINATOR) with the provided metadata name. This results in an inline capacity of 20 bytes being used for storage. In practice, the metadata names supplied in our calls (via meta_key.as_ref() in kv/mod.rs) tend to be short, and even if the metadata name itself exceeds the inline space (leaving roughly 12 bytes for the name), SmallVec will gracefully allocate on the heap.
There isn’t any evidence—in tests or usage—that metadata keys are regularly longer than expected or that there’s any performance or correctness issue with the present setup. We can always revisit this decision if profiling shows that a heap allocation occurs too frequently. For now, the inline capacity of 20 meets our needs.
server/websocket/src/bin/main.rs (3)
61-72: LGTM!The AppState implementation is clean and properly handles both authenticated and non-authenticated configurations.
91-95: Avoid logging sensitive authentication tokens.Logging authentication tokens in warnings can expose sensitive information and pose a security risk.
96-101: Return appropriate HTTP status codes in error responses.Error responses should return appropriate HTTP status codes to accurately reflect the error condition.
| let listener = tokio::net::TcpListener::bind(format!("0.0.0.0:{}", PORT)) | ||
| .await | ||
| .unwrap(); | ||
| axum::serve(listener, app).await.unwrap(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace unwrap() with proper error handling and add signal handling.
The current implementation uses unwrap() which can cause panics, and doesn't handle shutdown signals properly.
- let listener = tokio::net::TcpListener::bind(format!("0.0.0.0:{}", PORT))
- .await
- .unwrap();
- axum::serve(listener, app).await.unwrap();
+ let listener = tokio::net::TcpListener::bind(format!("0.0.0.0:{}", PORT))
+ .await
+ .map_err(|e| {
+ tracing::error!("Failed to bind to port {}: {}", PORT, e);
+ std::process::exit(1);
+ })?;
+
+ // Setup shutdown signal handler
+ let (tx, _) = tokio::sync::broadcast::channel(1);
+ let shutdown_signal = tx.clone();
+
+ tokio::spawn(async move {
+ let ctrl_c = async {
+ tokio::signal::ctrl_c()
+ .await
+ .expect("Failed to install Ctrl+C handler");
+ };
+
+ #[cfg(unix)]
+ let terminate = async {
+ tokio::signal::unix::signal(tokio::signal::unix::SignalKind::terminate())
+ .expect("Failed to install signal handler")
+ .recv()
+ .await;
+ };
+
+ #[cfg(not(unix))]
+ let terminate = std::future::pending::<()>();
+
+ tokio::select! {
+ _ = ctrl_c => {},
+ _ = terminate => {},
+ }
+
+ tracing::info!("Shutdown signal received");
+ let _ = shutdown_signal.send(());
+ });
+
+ axum::serve(listener, app)
+ .with_graceful_shutdown(async move {
+ let _ = tx.subscribe().recv().await;
+ })
+ .await
+ .map_err(|e| {
+ tracing::error!("Server error: {}", 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.
| let listener = tokio::net::TcpListener::bind(format!("0.0.0.0:{}", PORT)) | |
| .await | |
| .unwrap(); | |
| axum::serve(listener, app).await.unwrap(); | |
| let listener = tokio::net::TcpListener::bind(format!("0.0.0.0:{}", PORT)) | |
| .await | |
| .map_err(|e| { | |
| tracing::error!("Failed to bind to port {}: {}", PORT, e); | |
| std::process::exit(1); | |
| })?; | |
| // Setup shutdown signal handler | |
| let (tx, _) = tokio::sync::broadcast::channel(1); | |
| let shutdown_signal = tx.clone(); | |
| tokio::spawn(async move { | |
| let ctrl_c = async { | |
| tokio::signal::ctrl_c() | |
| .await | |
| .expect("Failed to install Ctrl+C handler"); | |
| }; | |
| #[cfg(unix)] | |
| let terminate = async { | |
| tokio::signal::unix::signal(tokio::signal::unix::SignalKind::terminate()) | |
| .expect("Failed to install signal handler") | |
| .recv() | |
| .await; | |
| }; | |
| #[cfg(not(unix))] | |
| let terminate = std::future::pending::<()>(); | |
| tokio::select! { | |
| _ = ctrl_c => {}, | |
| _ = terminate => {}, | |
| } | |
| tracing::info!("Shutdown signal received"); | |
| let _ = shutdown_signal.send(()); | |
| }); | |
| axum::serve(listener, app) | |
| .with_graceful_shutdown(async move { | |
| let _ = tx.subscribe().recv().await; | |
| }) | |
| .await | |
| .map_err(|e| { | |
| tracing::error!("Server error: {}", e); | |
| std::process::exit(1); | |
| })?; |
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
Improvements