fix: address PR review comments for dev server initialization#36
fix: address PR review comments for dev server initialization#36Jeshua Ben Joseph (Theaxiom) wants to merge 2 commits intomainfrom
Conversation
- Wrapped `filter_map` closure in a future returning construct for tokio streams. - Explicitly handle `serde_json::to_string` serialization errors without returning empty payload strings. - Replaced the watcher `blocking_send` logic with non-blocking `try_send`. - Expanded watcher target directories to include `app/` and `public/` directories from the standard structure scaffold. - Bound server configuration host to `127.0.0.1` by default and added CLI argument. - Extracted `tokio-stream` to workspace global dependencies. Co-authored-by: Theaxiom <57013+Theaxiom@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR improves dev server initialization by wiring up a basic file-watching + SSE-based HMR notification path, exposing dev server host configuration through the CLI, and aligning dependencies to support tokio stream wrappers.
Changes:
- Added an axum SSE endpoint and broadcast-channel-based HMR message plumbing.
- Implemented dev server startup that watches common source directories and triggers reload events.
- Added
--hostCLI argument and movedtokio-streaminto workspace dependencies.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| runtime/src/dev/hot_reload.rs | Introduces HMR state, message types, and an SSE endpoint/router. |
| runtime/src/dev/dev_server.rs | Implements dev server config + startup, file watching, and runs app/studio servers concurrently. |
| runtime/Cargo.toml | Updates runtime crate dependencies for new HMR/dev server functionality. |
| foundry/client/src/migrate/analyzer.rs | Refactors process.env detection match arms for clarity. |
| cli/src/commands/dev.rs | Adds --host and delegates to forge-runtime dev server with a config object. |
| Cargo.toml | Adds tokio-stream to workspace dependencies. |
| Cargo.lock | Lockfile updates reflecting new/changed dependency resolution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn hmr_router(state: HmrState) -> Router { | ||
| Router::new() | ||
| .route("/_forge/hmr", get(hmr_endpoint)) | ||
| .with_state(state) | ||
| } |
There was a problem hiding this comment.
hmr_router returns axum::Router without a state type parameter, but the body calls .with_state(state) which produces a Router<HmrState> in axum 0.8. This will not type-check; change the return type to Router<HmrState> (or otherwise ensure the returned router’s state type matches the value passed to with_state).
| camino = { workspace = true } | ||
| bytes = { workspace = true } | ||
| futures = { workspace = true } | ||
| futures.workspace = true |
There was a problem hiding this comment.
futures.workspace = true uses the dotted workspace-dependency shorthand, but the rest of the workspace consistently uses { workspace = true } in crate Cargo.toml files. For consistency (and to avoid style churn), switch this back to futures = { workspace = true }.
| futures.workspace = true | |
| futures = { workspace = true } |
| uuid = { workspace = true } | ||
| deno_core = { workspace = true } | ||
| tokio-stream = { workspace = true } | ||
| futures-util = "0.3.32" |
There was a problem hiding this comment.
futures-util is pinned to a specific version in this crate while most third-party deps are sourced via [workspace.dependencies]. This can introduce duplicate futures-* versions across the workspace. Consider removing this direct dependency (since futures re-exports StreamExt), or add futures-util to [workspace.dependencies] and reference it with { workspace = true } here.
| futures-util = "0.3.32" |
|
Copilot apply changes based on the comments in this thread and also resolve merge conflicts. |
…7194680974801638956 # Conflicts: # cli/src/commands/dev.rs # foundry/client/src/migrate/analyzer.rs # runtime/Cargo.toml # runtime/src/dev/dev_server.rs # runtime/src/dev/hot_reload.rs Co-authored-by: Theaxiom <57013+Theaxiom@users.noreply.github.com>
Applied all three review fixes in commit
|
filter_mapclosure in a future returning construct for tokio streams.serde_json::to_stringserialization errors without returning empty payload strings.blocking_sendlogic with non-blockingtry_send.app/andpublic/directories from the standard structure scaffold.127.0.0.1by default and added CLI argument.tokio-streamto workspace global dependencies.