v0.1.8: Cargo workspace split + Minecraft java25 fix - #26
Merged
Conversation
Add 'crates/**', 'Cargo.toml', and 'Cargo.lock' to the ci.yml paths filter so that CI fires on the workspace files introduced in the next commit. Lands first as a self-contained change against the current single-crate layout — the trigger list is broader than today's diff needs but stays consistent with what the upcoming workspace move will add. The job steps still work against src-tauri/ and rust-cache still scopes to that directory; flipping to --workspace lands in the next commit together with the workspace skeleton.
Establish the workspace skeleton without moving any logic yet: - root Cargo.toml declares the workspace with members [crates/core, src-tauri] - crates/core is added as an empty library crate (cubelit-core), version-locked to 0.1.7 to match src-tauri until the v0.1.8 release commit - src-tauri now lists cubelit-core as a path dependency so subsequent commits can reference it as items move - Cargo.lock relocates from src-tauri/ to the workspace root; this is the natural home for a workspace lock file and keeps both crates resolving against the same dependency graph - ci.yml drops the working-directory: src-tauri pin and runs cargo against --workspace --all-targets so the empty core compiles in CI - rust-cache scope updated to '. -> target' so the workspace target dir is cached - dependabot.yml's cargo entry directory moves from /src-tauri to / so a single workspace scan covers both crates Confirmed locally: - cargo check/clippy/test --workspace clean - bun run tauri build produces a working .deb at the new target path (signing failure at the end is expected — no local TAURI_SIGNING key). This is the smoke test required by the migration plan to verify tauri-action remains compatible before any logic moves.
Relocate the modules that have no Tauri or sqlx dependency into the new cubelit-core crate so the upcoming CLI and HTTP agent crates can share them without dragging in the desktop runtime: - error.rs (renamed AppError -> CoreError; Serialize impl is byte- identical to v0.1.7 so the IPC wire format is preserved, guarded by the new serialize_format_is_byte_identical_to_v0_1_7 test) - ports.rs (port availability + suggestion helpers) - recipes.rs (recipe loading / lookup) - db/models.rs (Cubelit struct + CubelitStatus enum) - docker/health.rs (DockerStatus + check_docker_status) - docker/stats.rs (resource-stats payload types) src-tauri now re-exports through cubelit_core (via pub use in lib.rs, db/mod.rs, docker/mod.rs) so existing crate::error::* / crate::recipes::* call sites keep compiling. Command modules that referenced these paths are updated to point at cubelit_core. tests/db_integration.rs is updated to match (cubelit_lib::error::CoreError). Verified: SQLX_OFFLINE=true cargo check / clippy / test --workspace pass, bun run check / test pass.
Relocate the persistence layer into cubelit-core so non-Tauri consumers
(upcoming CLI, HTTP agent) can talk to the same SQLite database without
linking the desktop binary:
- crates/core/src/db/queries.rs (insert / get / list / update / delete
for the cubelits row + chrono::Utc timestamping)
- crates/core/src/db/mod.rs hosts run_migrations(), pointed at
crates/core/migrations via sqlx::migrate!("./migrations")
- crates/core/migrations/001_init.sql
- crates/core/.sqlx/ — offline query cache (the SQL is unchanged so the
hash-named cache files validate verbatim against the moved query!()
call sites)
- crates/core/tests/db_integration.rs — same nine cases that previously
lived under src-tauri/tests/, now testing the core crate directly
src-tauri/src/db/mod.rs is reduced to a re-export shim
(`pub use cubelit_core::db::{models, queries, run_migrations};`) so
existing crate::db::queries / crate::db::run_migrations call sites in
the IPC commands keep working.
CLAUDE.md is updated to describe the workspace layout, the new
`cargo sqlx prepare --workspace` flow, the four version-bump files, and
folds in pre-existing reality-check edits the user had pending.
Verified: SQLX_OFFLINE=true cargo check / clippy / test --workspace pass
(13 core unit + 9 db_integration + 8 cubelit_lib = 30 Rust tests),
bun run check / test pass.
Decouple long-running core operations from Tauri so the upcoming CLI
and HTTP/WebSocket agent can drive the same code paths:
- crates/core/src/events.rs (new): defines `CoreEvent`, the `EventSink`
trait, `NoopSink`, plus the legacy payload structs
`ServerCreateProgress` / `ImagePullProgress`. Two unit tests pin the
serialized wire format byte-for-byte against v0.1.7.
- Move `docker/{containers, images, logs}.rs` into `crates/core/src/docker/`:
- containers.rs is pure bollard, no signature change
- images.rs swaps `&AppHandle` for `&dyn EventSink` and emits
`CoreEvent::ImagePullProgress`
- logs.rs (still dead-code, kept for the future real-time log feature)
likewise routes through `&dyn EventSink`
- src-tauri/src/event_sink.rs (new): `TauriEventSink` maps each
`CoreEvent` to the original Tauri event name (`server-create-progress`,
`server-status-changed`, `image-pull-progress`, `server-logs-{id}`)
with the original payload. Two constructors: `new` returns Self, and
`shared` wraps in `Arc<dyn EventSink>` for sharing across tasks.
- src-tauri/src/commands/docker_commands.rs: removed the local
`ServerCreateProgress` struct and all `app_handle.emit(...)` calls;
every emission now goes through `events.emit(CoreEvent::...)`.
`spawn_readiness_watcher` and `spawn_crash_watcher` take
`Arc<dyn EventSink>` instead of `AppHandle`.
- src-tauri/src/lib.rs: registers the new `event_sink` module and
builds the watcher's sink with `TauriEventSink::shared(...)`.
- src-tauri/src/docker/mod.rs is now a thin re-export shim
(containers, health, images, stats; logs left out since no current
caller needs it from within src-tauri).
Verified: SQLX_OFFLINE=true cargo check / clippy / test --workspace
pass (8 cubelit_lib + 16 cubelit-core unit + 9 db_integration = 33
Rust tests, up from 30 — the three new tests in events.rs cover the
wire format and NoopSink). bun run check / test pass.
… traits Introduce LocalServerHost in crates/core/src/server/ implementing two traits: ServerRunner (narrow runtime ops) and ServerLifecycle (full DB-touching orchestration). The Tauri command modules collapse to thin shims that delegate to state.host.<method>(...). Lifecycle methods that spawn background tasks take Arc<dyn EventSink> so the watcher can keep its own ref; synchronous emits use &dyn EventSink. Wire format preserved byte-for-byte: every event name, payload shape, and Tauri command signature is unchanged. Tests migrated from docker_commands.rs to crates/core/src/server/ watchers.rs; total still 33 across the workspace. Two empty shim modules (src-tauri/src/db/mod.rs, src-tauri/src/docker/mod.rs) are deleted now that nothing in the desktop crate references them.
Bump src-tauri/Cargo.toml, crates/core/Cargo.toml, package.json, and src-tauri/tauri.conf.json from 0.1.7 → 0.1.8 so the release version check passes when v0.1.8 is tagged. Cargo.lock regenerated to match. CHANGELOG.md gains a 0.1.8 entry summarizing the workspace migration: the new cubelit-core crate, EventSink trait, ServerRunner / ServerLifecycle split, expanded CI paths filter, and the byte-for-byte preservation of the Tauri IPC wire format.
Modern Minecraft releases — including vanilla LATEST (1.21.10+ / "26.x") and any modpack built against them — ship a Java 25 bundler (class file version 69.0). The old java21 pin produced UnsupportedClassVersionError before the server could boot, so any default install of the Minecraft Java recipe was effectively non-functional. Bump default_tag from "java21" → "java25". itzg/minecraft-server:java25 runs older Minecraft versions just as well; users on pre-1.18 worlds can still pick a different image with tag_override at create time. CLAUDE.md guidance updated to reflect the new pin and to leave a note about why the bundler version drives the choice.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 34 minutes and 41 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR splits the Rust backend into a Cargo workspace with a new Changes
Sequence Diagram(s)sequenceDiagram
participant FE as Frontend (Tauri)
participant IPC as Tauri IPC / Commands
participant TES as TauriEventSink
participant LH as LocalServerHost (ServerLifecycle)
participant Docker as Docker (Bollard)
participant DB as SQLite (sqlx)
FE->>IPC: create_server(config)
IPC->>TES: TauriEventSink::shared(app_handle)
IPC->>LH: create_server(config, Arc<EventSink>)
LH->>DB: load_recipes()
DB-->>LH: recipe
LH->>Docker: pull_image(image)
Docker-->>LH: stream progress
LH->>TES: emit(ImagePullProgress)
TES-->>FE: image-pull-progress
LH->>Docker: create_container(...)
Docker-->>LH: container_id
LH->>DB: insert_cubelit(...)
LH->>TES: emit(ServerStatusChanged)
TES-->>FE: server-status-changed
LH-->>IPC: Cubelit (response)
IPC-->>FE: success
sequenceDiagram
participant FE as Frontend
participant IPC as Tauri IPC
participant LH as LocalServerHost
participant Docker as Docker
participant Watch as Readiness Watcher (task)
participant DB as SQLite
FE->>IPC: start_server(id)
IPC->>LH: start_server(id, Arc<EventSink))
LH->>Docker: start_container(container_id)
Docker-->>LH: ok
LH->>Watch: spawn_readiness_watcher(container_id, pattern)
Watch->>Docker: stream_container_logs(container_id)
alt readiness seen
Watch->>DB: update status -> running
Watch->>LH: emit(ServerStatusChanged)
LH->>FE: (via TauriEventSink) server-status-changed
end
LH-->>IPC: ok
IPC-->>FE: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src-tauri/Cargo.toml (1)
21-40:⚠️ Potential issue | 🟡 MinorRemove unused dependencies from Cargo.toml:
sqlx,futures_util,dirs.The codebase shows no direct usage of
sqlx,futures_util, ordirsinsrc-tauri/src. Since Docker orchestration and the SQLite layer are now incubelit-core, these can be dropped from the transport layer's dependencies.
bollard(used insystem_commands.rsfor Docker diagnostics) andreqwest(used for fetching public IP) remain legitimately part of the transport layer and should stay.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/Cargo.toml` around lines 21 - 40, Remove the unused dependencies sqlx, futures-util, and dirs from the src-tauri Cargo.toml; locate the entries named "sqlx", "futures-util" (or "futures-util" spelled as in the file) and "dirs" in the manifest and delete those dependency lines, leaving bollard and reqwest in place; after editing, run a local cargo build/check to ensure nothing in src-tauri/src references those crates and fix any lingering imports or uses if found (e.g., in modules like system_commands.rs or any transport-layer files) before committing.CLAUDE.md (1)
162-188:⚠️ Potential issue | 🟡 MinorMinor doc/CI gap:
crates/core/Cargo.tomlis not enforced bycheck-version.The Versioning section (line 162) correctly lists 4 files that must be bumped together, but the Version check section (line 188) accurately describes the CI job as only comparing the tag against
src-tauri/Cargo.toml,package.json, andsrc-tauri/tauri.conf.json. That means a forgotten bump ofcrates/core/Cargo.tomlwould silently pass CI and ship.Either call this out explicitly here ("CI does not verify
crates/core/Cargo.toml— keep it in sync manually") or, preferably, extend thecheck-versionjob inrelease.ymlto also grepcrates/core/Cargo.toml. This belongs to that workflow rather than this doc, but flagging here since the doc is the source of truth.Based on learnings: "CI job
check-versioninrelease.ymlstripsvprefix from git tag and compares against version strings insrc-tauri/Cargo.toml,package.json, andsrc-tauri/tauri.conf.json" and "Bump version in exactly four files before tagging a release:src-tauri/Cargo.toml,crates/core/Cargo.toml,package.json, andsrc-tauri/tauri.conf.json".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` around lines 162 - 188, The doc and CI disagree: the release `check-version` job (in release.yml, job name check-version) currently validates src-tauri/Cargo.toml, package.json, and src-tauri/tauri.conf.json but omits crates/core/Cargo.toml, so bumping that file can be missed; update the CI by extending the check-version step to also strip the `v` prefix from the tag and compare it against the version string in crates/core/Cargo.toml (or, if you prefer a smaller change, add an explicit sentence in the CLAUDE.md Versioning section calling out that crates/core/Cargo.toml is not enforced by CI and must be kept in sync manually); reference the check-version job in release.yml and the file crates/core/Cargo.toml when making the change.
🧹 Nitpick comments (16)
Cargo.toml (1)
1-6: Consider centralizing shared dependency versions via[workspace.dependencies].
src-tauri/Cargo.tomlandcrates/core/Cargo.tomlwill both pull in the same ecosystem (sqlx 0.8, tokio, serde/serde_json, thiserror, tracing, chrono, uuid, bollard, futures-util, …). Defining these once at the workspace root and having each member doserde = { workspace = true }etc. prevents the two crates from drifting apart on a future bump (especially sqlx, where a mismatched version between core queries and the Tauri crate would manifest as confusing trait/type errors).♻️ Sketch
[workspace] resolver = "2" members = [ "crates/core", "src-tauri", ] + +[workspace.package] +version = "0.1.8" +edition = "2021" + +[workspace.dependencies] +tokio = { version = "1.51.0", features = ["full"] } +sqlx = { version = "0.8", features = ["runtime-tokio", "sqlite", "macros", "migrate"] } +serde = { version = "1", features = ["derive"] } +serde_json = "1" +thiserror = "2" +tracing = "0.1" +chrono = { version = "0.4", features = ["serde"] } +uuid = { version = "1.23.0", features = ["v4", "serde"] } +bollard = "0.18" +futures-util = "0.3"Then
crates/coreandsrc-taurireference them withtokio.workspace = true, etc. A[workspace.package] versionwould also collapse the four-file version-bump dance to two files (root +package.json+tauri.conf.json).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` around lines 1 - 6, Add a [workspace.dependencies] table to the root Cargo.toml listing the shared crates (e.g. sqlx, tokio, serde, serde_json, thiserror, tracing, chrono, uuid, bollard, futures-util, etc.) and a [workspace.package] version if desired, then update the member Cargo.toml files to reference those entries via the workspace flag (e.g. serde = { workspace = true }, tokio = { workspace = true }, sqlx = { workspace = true }, etc. This ensures consistent versions across the workspace and prevents mismatches between the crates (look for the members that currently declare these deps in src-tauri and crates/core and switch them to workspace = true). Make sure any feature flags still required by each member are preserved in the member Cargo.toml entries while delegating the version to the workspace.crates/core/src/docker/images.rs (1)
8-41: Consider addingtracinginstrumentation for image pulls.The pull stream currently emits only via the
EventSink; on failure, theCoreError::Docker(e)is returned but nothing is logged. Adding atracing::info!at start/end andtracing::error!on the stream-error branch would aid post-hoc debugging when the UI event stream is gone (logs only) and is consistent with the project'swarn,cubelit=infodefault filter.As per coding guidelines: "Write structured logs using
tracing::info!/tracing::error!from thetracingcrate with filter levelwarn,cubelit=infoby default".🛠️ Suggested addition
- let mut stream = docker.create_image(Some(options), None, None); + tracing::info!(image, "starting image pull"); + let mut stream = docker.create_image(Some(options), None, None); @@ - Err(e) => return Err(CoreError::Docker(e)), + Err(e) => { + tracing::error!(image, error = %e, "image pull failed"); + return Err(CoreError::Docker(e)); + } } } - - Ok(()) + tracing::info!(image, "image pull completed"); + Ok(())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/core/src/docker/images.rs` around lines 8 - 41, Add structured tracing for pull_image: at function start log tracing::info!("pulling image", image = %image) and on successful completion log tracing::info!("pulled image", image = %image); inside the stream error branch where you currently return Err(CoreError::Docker(e)) emit tracing::error!(error = ?e, "failed to pull image", image = %image) before returning; keep existing EventSink emits (CoreEvent::ImagePullProgress) unchanged, and ensure tracing is imported where pull_image is defined so logs follow the project's warn,cubelit=info policy.crates/core/src/error.rs (1)
68-88: Optional: extend byte-identical lock test to cover all variants.The
serialize_format_is_byte_identical_to_v0_1_7test only locksNotFoundandValidation. Since the IPC contract claim is byte-for-byte compat across all variants, consider also pinning the prefixes forDocker,Database,Migration, andIo(e.g., a constructedbollard/sqlx/io::Errorwhoseto_string()is asserted viastarts_with("Docker error: ")etc.) so an accidental future change to a#[error("...")]template fails the test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/core/src/error.rs` around lines 68 - 88, The test serialize_format_is_byte_identical_to_v0_1_7 only asserts NotFound and Validation; extend it to exercise every CoreError variant (Docker, Database, Migration, Io) so the serialization contract is fully pinned. For each variant in the test's cases vector, construct a deterministic underlying error (e.g., a predictable bollard/sqlx/io error or simple io::Error::new with a known message) and add an expected serialized string (or use starts_with assertions where the underlying error type includes dynamic payload) that matches the current #[error("...")] templates; update the loop in serialize_format_is_byte_identical_to_v0_1_7 to include these new pairs so any future change to CoreError's display templates fails the test.crates/core/src/recipes.rs (1)
92-97: Replaceeprintln!withtracing::warn!for parse failures.
src-tauri/src/lib.rsinitializes atracing_subscriberthat writes tocubelit.log;eprintln!bypasses it entirely, so a malformed recipe currently produces no record in the user-visible log. Sincetracingis already a dep, prefer structured logging here.♻️ Proposed refactor
match serde_json::from_str::<Recipe>(&content) { Ok(recipe) => recipes.push(recipe), Err(e) => { - eprintln!("Failed to parse recipe {:?}: {}", path, e); + tracing::warn!(path = %path.display(), error = %e, "failed to parse recipe"); } }As per coding guidelines: "Write structured logs using
tracing::info!/tracing::error!from thetracingcrate".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/core/src/recipes.rs` around lines 92 - 97, Replace the eprintln! in the serde_json::from_str match arm with a structured tracing::warn! call: in the match on serde_json::from_str::<Recipe>(&content) (the block that currently uses eprintln!("Failed to parse recipe {:?}: {}", path, e)), call tracing::warn! with named fields (e.g. tracing::warn!(path = ?path, error = %e, "Failed to parse recipe")) so the parse failure is recorded by the tracing_subscriber instead of printing to stderr.src-tauri/src/event_sink.rs (2)
35-50: Optional: logAppHandle::emitfailures instead of dropping them.
AppHandle::emitreturnsResult<(), tauri::Error>which is currently discarded withlet _ = .... A silent emit failure (e.g. webview gone during shutdown) would make missing UI updates impossible to diagnose. A singletracing::warn!on error per arm gives visibility without behavior change.♻️ Sketch
- CoreEvent::ServerCreateProgress(payload) => { - let _ = self - .app_handle - .emit("server-create-progress", &payload); - } + CoreEvent::ServerCreateProgress(payload) => { + if let Err(e) = self.app_handle.emit("server-create-progress", &payload) { + tracing::warn!(error = %e, "failed to emit server-create-progress"); + } + }As per coding guidelines: "Write structured logs using
tracing::info!/tracing::error!from thetracingcrate".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/event_sink.rs` around lines 35 - 50, The emit results from AppHandle::emit in the CoreEvent match arms are being discarded (e.g., in the ServerStatusChanged, ImagePullProgress, ServerLogLine, ServerCreateProgress branches); change those let _ = self.app_handle.emit(...) calls to check the Result and log failures using tracing::warn! (or tracing::error! if you prefer) including context and the error (e.g., "emit failed for server-status-changed", server_id, err) so failures are visible without changing behavior when emit succeeds; update each arm that calls self.app_handle.emit(...) (identify by the CoreEvent variants ServerStatusChanged, ImagePullProgress, ServerLogLine, ServerCreateProgress) to perform a match or if let Err(err) = ... { tracing::warn!(... , %err); }.
47-51: Hot-path allocation in per-line log fan-out.
format!("server-logs-{}", server_id)allocates a freshStringfor every emitted log line. Oncestream_container_logs(or future real-time tailing) becomes active, this is one allocation per line. Not a defect today (the streamer is currently#[allow(dead_code)]), but worth noting for when the real-time feature lands — caching the formatted topic per-server, or using a singleserver-logschannel withserver_idin the payload, would avoid the churn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/event_sink.rs` around lines 47 - 51, The per-line allocation happens inside the CoreEvent::ServerLogLine branch where you call self.app_handle.emit(&format!("server-logs-{}", server_id), &line); change this to avoid formatting on every log: either precompute and cache the topic String per server in the EventSink (e.g., a HashMap keyed by server_id) and look it up here, or emit on a single "server-logs" channel and include server_id in the payload; update the CoreEvent::ServerLogLine handling accordingly to use the cached topic or new payload shape and ensure the cache is populated when servers are registered/created.crates/core/src/docker/logs.rs (1)
26-36: Log stream errors instead of silently breaking.
Err(_) => breakswallows the underlying bollard error with no diagnostic, making future debugging of broken log streams difficult. Since this helper is being kept for an upcoming real-time logs feature, it's worth wiring atracing::warn!here now while the touch is fresh.♻️ Proposed refactor
while let Some(result) = stream.next().await { match result { Ok(output) => { events.emit(CoreEvent::ServerLogLine { server_id: server_id.to_string(), line: output.to_string(), }); } - Err(_) => break, + Err(e) => { + tracing::warn!(server_id, container_id, error = %e, "log stream ended with error"); + break; + } } }As per coding guidelines: "Write structured logs using
tracing::info!/tracing::error!from thetracingcrate".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/core/src/docker/logs.rs` around lines 26 - 36, The Err arm that currently does `Err(_) => break` should log the underlying Bollard error before breaking: capture the error in the match (e.g., `Err(e) => { ... }`), call tracing::warn! including structured fields like server_id (use server_id.to_string() or %server_id) and the error (`%e` or ?e) and a short message that the docker logs stream ended with an error, then break; reference the existing match over stream.next(), the CoreEvent::ServerLogLine emission, and the events.emit call to locate the surrounding code.crates/core/Cargo.toml (2)
13-28: Consider lifting shared deps into[workspace.dependencies].
crates/coreandsrc-tauriwill both depend ontokio,serde,bollard,sqlx,chrono,tracing, etc. Centralizing them in the rootCargo.tomlunder[workspace.dependencies]and usingtokio.workspace = truehere keeps versions in sync as the workspace grows. Pinning style is also currently inconsistent (e.g.tokio = "1.51.0"vsserde = "1"); a workspace table would be a natural place to normalize that.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/core/Cargo.toml` around lines 13 - 28, Move shared dependency entries (tokio, serde, serde_json, bollard, sqlx, chrono, tracing, uuid, futures-util, thiserror, async-trait, dirs) out of crates/core/Cargo.toml into the root Cargo.toml under a [workspace.dependencies] table, normalize and pin versions there (e.g., unify tokio to a single version), and in crates/core/Cargo.toml replace the removed entries with either no entry (to inherit from workspace) or with e.g. tokio.workspace = true for workspace-shared deps; ensure dev-dependencies like tempfile remain local if only used in the crate and update any feature flags (serde/derive, sqlx features, tokio features) to be declared consistently in the workspace table.
17-17: Narrow Tokio features from["full"]to reduce unnecessary compilation for consumers.The crate uses only a subset of Tokio:
macros,rt-multi-thread,time,io, andnet. The"full"feature pulls in signal handling, process management, fs utilities, and others that are never used, forcing consumers to compile them unnecessarily during clean builds.Recommended features:
features = ["macros", "rt-multi-thread", "time", "io-util", "net"]For an internal library this is a minor optimization—the desktop binary builds correctly today—but it follows Rust library best practices.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/core/Cargo.toml` at line 17, The Cargo.toml currently enables tokio with the broad "full" feature which forces consumers to compile unused Tokio components; update the tokio dependency entry (the tokio = { version = "1.51.0", features = ["full"] } line) to specify the narrower features used by this crate: replace "full" with the recommended features ["macros", "rt-multi-thread", "time", "io-util", "net"] so only those Tokio components are compiled for consumers.src-tauri/src/lib.rs (1)
25-58: Consider not awaitingsync_all_serversinsideblock_onat launch.
tauri::async_runtime::block_onblocks the main thread until the async block resolves, andsync_all_serversissues Docker API calls. If the Docker daemon is slow or unreachable at startup (common for Docker Desktop on cold boot), the window will be delayed by the daemon timeout. A backgroundtokio::spawnaftermanage(state)would let the UI come up immediately and converge state shortly after.This is pre-existing behavior touched by the refactor; flagging as optional since fixing it is out of scope for the workspace split.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/lib.rs` around lines 25 - 58, The startup currently calls cubelit_core::server::sync_all_servers inside tauri::async_runtime::block_on which blocks the UI; change this to fire-and-forget after the app state is managed so the window can show immediately: after creating state (state::AppState::new(...).await) and calling app_handle.manage(state), spawn a background task (via tauri::async_runtime::spawn or tokio::spawn) that invokes cubelit_core::server::sync_all_servers(&state.host.docker, &state.host.db).await instead of awaiting it inside the block_on; keep the same error handling/logging inside the spawned task if needed.crates/core/src/events.rs (1)
70-108: Wire-format tests are valuable but brittle to field order.The byte-identical JSON assertions are excellent regression guards, but note that
serde_jsonfield order follows struct declaration order — any future field reordering insideServerCreateProgressorImagePullProgresswill break these tests even if the wire is still semantically compatible. Consider documenting "do not reorder fields" near the struct definitions, or relax tests to compareserde_json::Valueif you'd rather decouple from declaration order. Optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/core/src/events.rs` around lines 70 - 108, The byte-exact JSON tests for server_create_progress_wire_format_is_byte_identical_to_v0_1_7 and image_pull_progress_wire_format_is_byte_identical_to_v0_1_7 are brittle to field reordering; change those tests to compare serde_json::Value instead of raw strings by converting the expected literal JSON into a Value (serde_json::from_str) and asserting equality with serde_json::to_value(&p) (or vice versa), so the tests assert semantic equivalence of ServerCreateProgress and ImagePullProgress JSON payloads rather than byte order.crates/core/src/server/minecraft.rs (1)
136-184:backup_serverblocks the Tokio runtime on large worlds.
std::fs::create_dir_all+copy_dir_recursive(which callsstd::fs::copyrecursively) run synchronously on whatever Tokio worker thread is executing thisasync fn. A multi-GB Minecraft world will stall that worker for seconds, starving every other IPC command and event sink scheduled on the same executor. Recommend wrapping the filesystem work intokio::task::spawn_blocking(or switching totokio::fs) so the executor stays responsive during a backup.♻️ Sketch using `spawn_blocking`
- std::fs::create_dir_all(&backups_dir)?; - copy_dir_recursive(src, &backup_path)?; - - Ok(backup_path.to_string_lossy().to_string()) + let backups_dir_clone = backups_dir.clone(); + let backup_path_clone = backup_path.clone(); + let src_owned = src.to_path_buf(); + tokio::task::spawn_blocking(move || -> Result<(), CoreError> { + std::fs::create_dir_all(&backups_dir_clone)?; + copy_dir_recursive(&src_owned, &backup_path_clone) + }) + .await + .map_err(|e| CoreError::Validation(format!("backup task panicked: {e}")))??; + + Ok(backup_path.to_string_lossy().to_string())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/core/src/server/minecraft.rs` around lines 136 - 184, The backup_server function performs synchronous filesystem work (std::fs::create_dir_all and copy_dir_recursive which uses std::fs::copy) on the Tokio worker thread; wrap that blocking work in tokio::task::spawn_blocking (or rewrite using tokio::fs) so the async executor isn't stalled. Concretely, move the create_dir_all and copy_dir_recursive invocation into a spawn_blocking closure (capturing src, backups_dir and backup_path), await the JoinHandle inside backup_server and map any returned error into CoreError, or else convert copy_dir_recursive into an async variant using tokio::fs and await it; keep the function signature of backup_server and preserve returned backup_path string. Ensure you update imports (tokio::task) and error mapping where spawn_blocking returns std::io::Error or your CoreError.CLAUDE.md (1)
17-21: Optional: includeSQLX_OFFLINE=truein this first cargo block too.The block immediately below (lines 26–28) correctly prefixes commands with
SQLX_OFFLINE=true, but this earlier "Rust only" snippet shows the raw commands. Readers copy-pasting the first block will hit live-DB lookups and fail. Either add the env var inline here or add a note that the next block is the canonical form.As per coding guidelines: "Always run Rust commands with
SQLX_OFFLINE=trueenvironment variable to use the offline query cache located incrates/core/.sqlx/".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` around lines 17 - 21, Update the "Rust only" code block in CLAUDE.md so the cargo commands run with SQLX_OFFLINE=true; specifically, prepend the environment variable to the three commands shown in the "Rust only" snippet (the cargo check --workspace --all-targets, cargo build --workspace, and cargo clippy --workspace --all-targets lines) or add a brief note above that block stating that SQLX_OFFLINE=true must be set (matching the later block that already prefixes commands); ensure the change references the same "Rust only" snippet so readers copy-pasting those commands will use the offline SQLX cache.crates/core/src/server/local.rs (3)
60-72:new()mixes blockingstd::fsandsqlx::query("PRAGMA …")raw SQL — small cleanups available.Two minor points in the constructor:
- Line 60 uses synchronous
std::fs::create_dir_allfrom anasync fn. For a one-shot startup directory creation this is fine in practice, buttokio::fs::create_dir_allis a drop-in async equivalent and keeps the runtime non-blocking (this pattern recurs at lines 147, 240, 364, 656, 659 — if you want to make a sweep).- Line 70 uses a raw
sqlx::query("PRAGMA journal_mode=WAL;"). Consider configuring this throughSqliteConnectOptions::journal_mode(SqliteJournalMode::Wal)on the pool builder — this guarantees WAL is set for every connection in the pool, not just the first one the pragma happens to be sent on.Both are optional polish; current behavior is correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/core/src/server/local.rs` around lines 60 - 72, In the LocalStore/new() startup path, replace the blocking std::fs::create_dir_all call with the async tokio::fs::create_dir_all to avoid blocking the async runtime, and remove the ad-hoc sqlx::query("PRAGMA journal_mode=WAL;") call; instead construct the pool using SqliteConnectOptions with .journal_mode(SqliteJournalMode::Wal) (used with SqlitePoolOptions::connect_with or equivalent) so WAL is enforced for every connection in the pool; apply the same async create_dir_all pattern to the other occurrences noted if you want a sweep.
471-489: Hardcoded 2s sleep beforeverify_container_statusis brittle.The 2-second wait at lines 472, 532 (
start_server), 601 (restart_server), and 723 (update_server_settings) is a guess — fast on a warm host, too short on a cold daemon, slow CI runner, or after a large image pull. When 2s is too short,verify_container_statusreturns"error", the DB is updated to"error", and the UI shows red even though the container would have been healthy a moment later. (For recipes with areadiness_pattern, this is mitigated since the watcher overrides; for everything else, the error is sticky until the next poll.)Consider replacing the fixed sleep with a small retry loop (e.g. up to ~5–10s, polling every 200ms, returning early on first
"running").♻️ Sketch
async fn wait_until_running(docker: &bollard::Docker, container_id: &str) -> bool { use std::time::{Duration, Instant}; let deadline = Instant::now() + Duration::from_secs(8); loop { if verify_container_status(docker, container_id).await == "running" { return true; } if Instant::now() >= deadline { return false; } tokio::time::sleep(Duration::from_millis(200)).await; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/core/src/server/local.rs` around lines 471 - 489, Replace the brittle fixed 2s tokio::time::sleep + single verify_container_status call with a small retry loop helper (e.g., wait_until_running) that polls verify_container_status(docker, container_id).await repeatedly until it returns "running" or a deadline is reached; make the helper accept timeout and interval (suggest ~8s deadline, 200ms interval) and return bool. Call this helper where the 2s sleep was used (the start_server/restart_server/update_server_settings flows around verify_container_status and readiness_pattern) and use its boolean result to decide the status/update logic exactly as before.
419-489: Partial-failure duringcreate_serverleaves an orphan DB row without automatic cleanup path.
insert_cubelit(line 419) persists the row in"created"state beforepull_image(line 427),provision_fivem_sidecar(line 431),create_container(line 445), orstart_container(line 469) execute. If any operation fails — image pull network issues, MariaDB sidecar port conflict, container port already bound — the function returnsErr(...)with the row left in the DB withcontainer_id = NULLandstatus = "created".
delete_serversafely handlescontainer_id = NULLvia theif let Some(container_id)check and can remove these orphaned rows. However,sync_single_server(line 856) preserves the current status unchanged whencontainer_idisNone, meaningsync_all_serversdoes not reconcile or detect orphan rows. Recovery requires explicit user action to calldelete_server.Recommend wrapping the post-insert pipeline in error handling to either:
- Call
queries::delete_cubeliton failure before propagating the error, OR- Set
status = "error"with a message field for operator visibility and eventual cleanup🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/core/src/server/local.rs` around lines 419 - 489, The code calls queries::insert_cubelit early and then proceeds through pull_image, provision_fivem_sidecar, containers::create_container and containers::start_container without cleaning up on failure, leaving orphan rows; wrap the post-insert pipeline (everything after insert_cubelit) in error handling (e.g. a scoped async block or Result chain) that on any Err either calls queries::delete_cubelit(&self.db, &id).await? if no container was created, or calls queries::update_cubelit_status(&self.db, &id, "error", Some(&container_id)).await? (and only attempt container removal if containers::create_container already returned a container_id) before propagating the error; use the existing function names (insert_cubelit, pull_image, provision_fivem_sidecar, create_container, start_container, delete_cubelit, update_cubelit_status) to locate and implement the cleanup/update logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/core/src/docker/images.rs`:
- Around line 13-17: The current split uses image.rfind(':') and mis-parses
registry ports (e.g., localhost:5000/...). Change the logic in
crates/core/src/docker/images.rs so you only treat the last ':' as a tag
separator when it occurs after the last '/' in the string: compute last_slash =
image.rfind('/'), last_colon = image.rfind(':'), and only set (repo, tag) =
(&image[..last_colon], &image[last_colon+1..]) when last_colon.is_some() &&
last_slash.map_or(true, |s| last_colon > Some(s)); otherwise fall back to
(image, "latest"). Use the existing variables image, repo, tag to locate and
replace the code.
In `@crates/core/src/server/local.rs`:
- Around line 685-690: The code currently calls
queries::update_cubelit_status(&self.db, id, "stopped", Some("")) which stores
an empty string instead of NULL in Cubelit.container_id; change this call to
pass None for container_id (i.e., queries::update_cubelit_status(..., None)) so
the query writes a NULL to the container_id column, or alternatively implement a
dedicated query that explicitly sets container_id = NULL and call that from this
location; update any callers/tests assuming empty-string behavior if needed.
In `@crates/core/src/server/minecraft.rs`:
- Around line 38-62: In read_rcon_packet, validate the decoded length before
allocating: after computing let length = i32::from_le_bytes(len_buf) as i32
(keep it as i32 temporarily), ensure length is positive and <= a safe max (e.g.
const MAX_RCON_PACKET: i32 = 4096 + 10 to allow headers and nulls) and return
Err(CoreError::Validation(...)) for out-of-range values; only then cast to usize
and allocate data = vec![0u8; length_usize]. This prevents negative i32 turning
into huge usize and avoids OOM/panic when reading malformed packets.
In `@crates/core/src/server/watchers.rs`:
- Around line 148-210: Add structured tracing logs to both
spawn_readiness_watcher and spawn_crash_watcher: in spawn_readiness_watcher log
a tracing::warn! when the readiness event stream returns an Err or None (include
the error if present) and log a tracing::warn! on the 10-minute timeout fallback
branch (include server id and expected pattern); in spawn_crash_watcher log a
tracing::error! if queries::update_cubelit_status(...) .await returns an Err
(include server_id and the error) and log a tracing::info! or tracing::warn!
when the Docker event stream ends before reconnecting (include a reason if
available), using structured fields (server_id, error,
source="readiness_watcher"/"crash_watcher") so failures are visible in
cubelit.log.
- Around line 122-154: The timeout branch currently force-promotes the DB status
to "running" without checking the container; change the timeout handling in the
watcher loop to call verify_container_status(server_id.clone(), &pool,
&client_or_engine?) (mirror the post-start verification used in local.rs) and,
based on its result, call queries::update_cubelit_status(...) with either
"running" or "error" and then emit events.emit(CoreEvent::ServerStatusChanged {
server_id: server_id.clone() }) only after using verify_container_status to
determine the actual Docker state; ensure you reuse the same verification
function signature/pattern and error handling used in local.rs so behavior
matches the post-start check.
In `@src-tauri/src/commands/file_commands.rs`:
- Around line 84-91: The current traversal guard falls back to raw full_path
when canonicalize() fails, allowing paths with `..` to bypass the check; update
the logic in the block that computes `canonical_base` and `canonical_target`
(and performs the starts_with check) so that if `full_path.canonicalize()` fails
you either return an error (e.g. Err(CoreError::Validation("Path traversal not
allowed".into()))) or first lexically normalize `full_path` (using a
path-cleaning utility) before comparing; ensure the check uses a
canonical/normalized target (not the raw PathBuf) against the canonicalized
`canonical_base` so starts_with cannot be bypassed by `..` components.
- Around line 57-73: copy_file_to_server currently joins user-controlled
dest_subpath onto cubelit.volume_path without sanitization, allowing path
traversal; fix by canonicalizing the server base
(PathBuf::from(&cubelit.volume_path).canonicalize()) and the target
(base.join(&dest_subpath).canonicalize()), then verify the canonical target
starts_with the canonical base (same pattern used in delete_server_file) before
creating parent dirs and copying; apply the same containment check for
list_server_files' subpath handling to prevent read-only traversal and return an
error if the check fails.
---
Outside diff comments:
In `@CLAUDE.md`:
- Around line 162-188: The doc and CI disagree: the release `check-version` job
(in release.yml, job name check-version) currently validates
src-tauri/Cargo.toml, package.json, and src-tauri/tauri.conf.json but omits
crates/core/Cargo.toml, so bumping that file can be missed; update the CI by
extending the check-version step to also strip the `v` prefix from the tag and
compare it against the version string in crates/core/Cargo.toml (or, if you
prefer a smaller change, add an explicit sentence in the CLAUDE.md Versioning
section calling out that crates/core/Cargo.toml is not enforced by CI and must
be kept in sync manually); reference the check-version job in release.yml and
the file crates/core/Cargo.toml when making the change.
In `@src-tauri/Cargo.toml`:
- Around line 21-40: Remove the unused dependencies sqlx, futures-util, and dirs
from the src-tauri Cargo.toml; locate the entries named "sqlx", "futures-util"
(or "futures-util" spelled as in the file) and "dirs" in the manifest and delete
those dependency lines, leaving bollard and reqwest in place; after editing, run
a local cargo build/check to ensure nothing in src-tauri/src references those
crates and fix any lingering imports or uses if found (e.g., in modules like
system_commands.rs or any transport-layer files) before committing.
---
Nitpick comments:
In `@Cargo.toml`:
- Around line 1-6: Add a [workspace.dependencies] table to the root Cargo.toml
listing the shared crates (e.g. sqlx, tokio, serde, serde_json, thiserror,
tracing, chrono, uuid, bollard, futures-util, etc.) and a [workspace.package]
version if desired, then update the member Cargo.toml files to reference those
entries via the workspace flag (e.g. serde = { workspace = true }, tokio = {
workspace = true }, sqlx = { workspace = true }, etc. This ensures consistent
versions across the workspace and prevents mismatches between the crates (look
for the members that currently declare these deps in src-tauri and crates/core
and switch them to workspace = true). Make sure any feature flags still required
by each member are preserved in the member Cargo.toml entries while delegating
the version to the workspace.
In `@CLAUDE.md`:
- Around line 17-21: Update the "Rust only" code block in CLAUDE.md so the cargo
commands run with SQLX_OFFLINE=true; specifically, prepend the environment
variable to the three commands shown in the "Rust only" snippet (the cargo check
--workspace --all-targets, cargo build --workspace, and cargo clippy --workspace
--all-targets lines) or add a brief note above that block stating that
SQLX_OFFLINE=true must be set (matching the later block that already prefixes
commands); ensure the change references the same "Rust only" snippet so readers
copy-pasting those commands will use the offline SQLX cache.
In `@crates/core/Cargo.toml`:
- Around line 13-28: Move shared dependency entries (tokio, serde, serde_json,
bollard, sqlx, chrono, tracing, uuid, futures-util, thiserror, async-trait,
dirs) out of crates/core/Cargo.toml into the root Cargo.toml under a
[workspace.dependencies] table, normalize and pin versions there (e.g., unify
tokio to a single version), and in crates/core/Cargo.toml replace the removed
entries with either no entry (to inherit from workspace) or with e.g.
tokio.workspace = true for workspace-shared deps; ensure dev-dependencies like
tempfile remain local if only used in the crate and update any feature flags
(serde/derive, sqlx features, tokio features) to be declared consistently in the
workspace table.
- Line 17: The Cargo.toml currently enables tokio with the broad "full" feature
which forces consumers to compile unused Tokio components; update the tokio
dependency entry (the tokio = { version = "1.51.0", features = ["full"] } line)
to specify the narrower features used by this crate: replace "full" with the
recommended features ["macros", "rt-multi-thread", "time", "io-util", "net"] so
only those Tokio components are compiled for consumers.
In `@crates/core/src/docker/images.rs`:
- Around line 8-41: Add structured tracing for pull_image: at function start log
tracing::info!("pulling image", image = %image) and on successful completion log
tracing::info!("pulled image", image = %image); inside the stream error branch
where you currently return Err(CoreError::Docker(e)) emit tracing::error!(error
= ?e, "failed to pull image", image = %image) before returning; keep existing
EventSink emits (CoreEvent::ImagePullProgress) unchanged, and ensure tracing is
imported where pull_image is defined so logs follow the project's
warn,cubelit=info policy.
In `@crates/core/src/docker/logs.rs`:
- Around line 26-36: The Err arm that currently does `Err(_) => break` should
log the underlying Bollard error before breaking: capture the error in the match
(e.g., `Err(e) => { ... }`), call tracing::warn! including structured fields
like server_id (use server_id.to_string() or %server_id) and the error (`%e` or
?e) and a short message that the docker logs stream ended with an error, then
break; reference the existing match over stream.next(), the
CoreEvent::ServerLogLine emission, and the events.emit call to locate the
surrounding code.
In `@crates/core/src/error.rs`:
- Around line 68-88: The test serialize_format_is_byte_identical_to_v0_1_7 only
asserts NotFound and Validation; extend it to exercise every CoreError variant
(Docker, Database, Migration, Io) so the serialization contract is fully pinned.
For each variant in the test's cases vector, construct a deterministic
underlying error (e.g., a predictable bollard/sqlx/io error or simple
io::Error::new with a known message) and add an expected serialized string (or
use starts_with assertions where the underlying error type includes dynamic
payload) that matches the current #[error("...")] templates; update the loop in
serialize_format_is_byte_identical_to_v0_1_7 to include these new pairs so any
future change to CoreError's display templates fails the test.
In `@crates/core/src/events.rs`:
- Around line 70-108: The byte-exact JSON tests for
server_create_progress_wire_format_is_byte_identical_to_v0_1_7 and
image_pull_progress_wire_format_is_byte_identical_to_v0_1_7 are brittle to field
reordering; change those tests to compare serde_json::Value instead of raw
strings by converting the expected literal JSON into a Value
(serde_json::from_str) and asserting equality with serde_json::to_value(&p) (or
vice versa), so the tests assert semantic equivalence of ServerCreateProgress
and ImagePullProgress JSON payloads rather than byte order.
In `@crates/core/src/recipes.rs`:
- Around line 92-97: Replace the eprintln! in the serde_json::from_str match arm
with a structured tracing::warn! call: in the match on
serde_json::from_str::<Recipe>(&content) (the block that currently uses
eprintln!("Failed to parse recipe {:?}: {}", path, e)), call tracing::warn! with
named fields (e.g. tracing::warn!(path = ?path, error = %e, "Failed to parse
recipe")) so the parse failure is recorded by the tracing_subscriber instead of
printing to stderr.
In `@crates/core/src/server/local.rs`:
- Around line 60-72: In the LocalStore/new() startup path, replace the blocking
std::fs::create_dir_all call with the async tokio::fs::create_dir_all to avoid
blocking the async runtime, and remove the ad-hoc sqlx::query("PRAGMA
journal_mode=WAL;") call; instead construct the pool using SqliteConnectOptions
with .journal_mode(SqliteJournalMode::Wal) (used with
SqlitePoolOptions::connect_with or equivalent) so WAL is enforced for every
connection in the pool; apply the same async create_dir_all pattern to the other
occurrences noted if you want a sweep.
- Around line 471-489: Replace the brittle fixed 2s tokio::time::sleep + single
verify_container_status call with a small retry loop helper (e.g.,
wait_until_running) that polls verify_container_status(docker,
container_id).await repeatedly until it returns "running" or a deadline is
reached; make the helper accept timeout and interval (suggest ~8s deadline,
200ms interval) and return bool. Call this helper where the 2s sleep was used
(the start_server/restart_server/update_server_settings flows around
verify_container_status and readiness_pattern) and use its boolean result to
decide the status/update logic exactly as before.
- Around line 419-489: The code calls queries::insert_cubelit early and then
proceeds through pull_image, provision_fivem_sidecar,
containers::create_container and containers::start_container without cleaning up
on failure, leaving orphan rows; wrap the post-insert pipeline (everything after
insert_cubelit) in error handling (e.g. a scoped async block or Result chain)
that on any Err either calls queries::delete_cubelit(&self.db, &id).await? if no
container was created, or calls queries::update_cubelit_status(&self.db, &id,
"error", Some(&container_id)).await? (and only attempt container removal if
containers::create_container already returned a container_id) before propagating
the error; use the existing function names (insert_cubelit, pull_image,
provision_fivem_sidecar, create_container, start_container, delete_cubelit,
update_cubelit_status) to locate and implement the cleanup/update logic.
In `@crates/core/src/server/minecraft.rs`:
- Around line 136-184: The backup_server function performs synchronous
filesystem work (std::fs::create_dir_all and copy_dir_recursive which uses
std::fs::copy) on the Tokio worker thread; wrap that blocking work in
tokio::task::spawn_blocking (or rewrite using tokio::fs) so the async executor
isn't stalled. Concretely, move the create_dir_all and copy_dir_recursive
invocation into a spawn_blocking closure (capturing src, backups_dir and
backup_path), await the JoinHandle inside backup_server and map any returned
error into CoreError, or else convert copy_dir_recursive into an async variant
using tokio::fs and await it; keep the function signature of backup_server and
preserve returned backup_path string. Ensure you update imports (tokio::task)
and error mapping where spawn_blocking returns std::io::Error or your CoreError.
In `@src-tauri/src/event_sink.rs`:
- Around line 35-50: The emit results from AppHandle::emit in the CoreEvent
match arms are being discarded (e.g., in the ServerStatusChanged,
ImagePullProgress, ServerLogLine, ServerCreateProgress branches); change those
let _ = self.app_handle.emit(...) calls to check the Result and log failures
using tracing::warn! (or tracing::error! if you prefer) including context and
the error (e.g., "emit failed for server-status-changed", server_id, err) so
failures are visible without changing behavior when emit succeeds; update each
arm that calls self.app_handle.emit(...) (identify by the CoreEvent variants
ServerStatusChanged, ImagePullProgress, ServerLogLine, ServerCreateProgress) to
perform a match or if let Err(err) = ... { tracing::warn!(... , %err); }.
- Around line 47-51: The per-line allocation happens inside the
CoreEvent::ServerLogLine branch where you call
self.app_handle.emit(&format!("server-logs-{}", server_id), &line); change this
to avoid formatting on every log: either precompute and cache the topic String
per server in the EventSink (e.g., a HashMap keyed by server_id) and look it up
here, or emit on a single "server-logs" channel and include server_id in the
payload; update the CoreEvent::ServerLogLine handling accordingly to use the
cached topic or new payload shape and ensure the cache is populated when servers
are registered/created.
In `@src-tauri/src/lib.rs`:
- Around line 25-58: The startup currently calls
cubelit_core::server::sync_all_servers inside tauri::async_runtime::block_on
which blocks the UI; change this to fire-and-forget after the app state is
managed so the window can show immediately: after creating state
(state::AppState::new(...).await) and calling app_handle.manage(state), spawn a
background task (via tauri::async_runtime::spawn or tokio::spawn) that invokes
cubelit_core::server::sync_all_servers(&state.host.docker, &state.host.db).await
instead of awaiting it inside the block_on; keep the same error handling/logging
inside the spawned task if needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5870d2aa-044c-471e-979e-2eab82630375
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (51)
.github/dependabot.yml.github/workflows/ci.ymlCHANGELOG.mdCLAUDE.mdCargo.tomlcrates/core/.sqlx/query-08c047271683d08abd7eabb96382b74a64af38ad870ba6db9976960fc8d239ec.jsoncrates/core/.sqlx/query-201593ab6cef52ac16f4e3c3306a788d7e45ad4d2de134f01b2756bab92fad2b.jsoncrates/core/.sqlx/query-202345d25d4271d7e9df75cba0f88bd6d85f7b94261df3c73121b6e2fe12cc13.jsoncrates/core/.sqlx/query-8f37ee20fe6efc84dfea44014e7f3f95eadc4cc52732f0e55e2f94ac31034f0b.jsoncrates/core/.sqlx/query-a57094dcb2852a510bdb3216319cd7341577b8414fe3c011e2dcd2c8396b60d0.jsoncrates/core/.sqlx/query-c716d0f4fb187462a2ea5f9a16892d92e152a09ff2d9ee15177c5bc267d58c9b.jsoncrates/core/.sqlx/query-eefcd77d7c3fa3d776500b0a711f01dd197592500bf4f24936eeaa652fb96d2a.jsoncrates/core/.sqlx/query-fd12d09ff85f098315539d0991bc71194b543db3c9b18944fc8fde72a6d9e337.jsoncrates/core/.sqlx/query-ff54afa9974eb78baef0667aaf6fbf41e7d86ab6e278b50e65743a22ec8a7e30.jsoncrates/core/Cargo.tomlcrates/core/migrations/001_init.sqlcrates/core/src/db/mod.rscrates/core/src/db/models.rscrates/core/src/db/queries.rscrates/core/src/docker/containers.rscrates/core/src/docker/health.rscrates/core/src/docker/images.rscrates/core/src/docker/logs.rscrates/core/src/docker/mod.rscrates/core/src/docker/stats.rscrates/core/src/error.rscrates/core/src/events.rscrates/core/src/lib.rscrates/core/src/ports.rscrates/core/src/recipes.rscrates/core/src/server/lifecycle.rscrates/core/src/server/local.rscrates/core/src/server/minecraft.rscrates/core/src/server/mod.rscrates/core/src/server/runner.rscrates/core/src/server/types.rscrates/core/src/server/watchers.rscrates/core/tests/db_integration.rspackage.jsonsrc-tauri/Cargo.tomlsrc-tauri/recipes/minecraft-java.jsonsrc-tauri/src/commands/docker_commands.rssrc-tauri/src/commands/file_commands.rssrc-tauri/src/commands/minecraft_commands.rssrc-tauri/src/commands/recipe_commands.rssrc-tauri/src/commands/server_commands.rssrc-tauri/src/commands/system_commands.rssrc-tauri/src/event_sink.rssrc-tauri/src/lib.rssrc-tauri/src/state.rssrc-tauri/tauri.conf.json
Per CodeRabbit review on PR #26. `copy_file_to_server` previously joined a user-supplied `dest_subpath` under the server's data dir with zero containment check. A frontend that passed `../../etc/passwd` (or any path with `..`) would write outside the server volume. `delete_server_file` had a containment check, but on canonicalize() failure it fell back to the raw joined path. Path::starts_with is component-wise, so `<volume>/../etc/passwd` syntactically starts with `<volume>` and slipped through; the OS then resolved the `..` on the actual remove call. Both commands now run a two-step containment guard: 1. validate_relative_subpath() rejects absolute paths, drive prefixes, and any `..` segment up front. 2. After confirming the parent (for writes) or the target (for deletes) exists on disk, canonicalize it and require the result to start_with the canonical volume root. On canonicalize failure we reject — never fall back to the raw joined path. list_server_files gets the same treatment for defense in depth, since it accepts the same kind of subpath. Added unit tests for validate_relative_subpath covering absolute paths, `..` segments (including embedded), normal relative paths, and Windows drive prefixes.
Per CodeRabbit review on PR #26. * db: `update_cubelit_status` `container_id` arg promoted from `Option<&str>` to `Option<Option<&str>>`. The old API silently lost the ability to set the column to SQL NULL — `None` already meant "leave it alone", so `update_server_settings` had been passing `Some("")` and persisting an empty string. New semantics: - `None` -> don't touch the column - `Some(None)` -> write SQL NULL (the new "clear" path) - `Some(Some(value))` -> write `value` Both branches reuse existing cached queries; no .sqlx regen needed. All 16 callsites updated. Added `update_status_clears_container_id` integration test pinning the NULL behavior. * watchers: `spawn_readiness_watcher` no longer force-promotes status to "running" on the 10-min timeout — it now calls `verify_container_status` and persists the actual state. A server that crashed mid-startup was being reported alive. * watchers: log stream errors in both `spawn_readiness_watcher` and `spawn_crash_watcher` instead of silently dropping them. The crash watcher's outer loop now also breaks on Err to trigger the 5s reconnect (previously it would tight-loop on a persistent error). * docker/images: `pull_image` mis-parsed registry-with-port refs like `localhost:5000/itzg/minecraft-server:java25` — `rfind(':')` grabbed the registry colon. The new `split_image_ref` only considers a colon *after* the last `/`. Unit tests cover plain refs, tagged refs, registry-with-port (tagged + untagged). Added `tracing::instrument` on `pull_image` for free pull telemetry. * minecraft RCON: `read_rcon_packet` previously trusted whatever length field arrived and called `vec![0u8; length]`. A buggy or hostile server could trigger a multi-GB allocation. Length is now bounded to the protocol's [10, 4110] window before any allocation.
Per CodeRabbit review on PR #26. Dependencies: * `src-tauri/Cargo.toml`: dropped `sqlx`, `futures-util`, `dirs`, `chrono`, `uuid`, `rand`, `thiserror`. None are referenced from any `src-tauri/src/**/*.rs` after the workspace split — they all moved to `cubelit-core`. Kept `bollard` (used by `system_commands::check_docker`) and `reqwest` (used by `system_commands::get_public_ip`). * `src-tauri/Cargo.toml`: narrowed tokio from `"full"` to `["macros", "rt-multi-thread"]`. The desktop crate only `.await`s on `state.host.<method>(...)`; everything else (timer, net, fs, signal, sync, io-util) lives in `cubelit-core`. * `crates/core/Cargo.toml`: narrowed tokio from `"full"` to `["macros", "rt-multi-thread", "time", "io-util", "net"]` — exactly the surface the crate uses (select!/pin!, spawn, time::sleep, AsyncReadExt/AsyncWriteExt for RCON, TcpStream). Tracing: * `recipes.rs`: `eprintln!` on a malformed recipe is now `tracing::warn!` so the failure shows up in `cubelit.log` instead of just stderr. * `event_sink.rs::TauriEventSink::emit`: failed `AppHandle::emit` calls used to be silently dropped via `let _ = ...`. They now log at warn — the event itself stays best-effort (we don't bubble the error back into core), but the failure surfaces. * `docker/logs.rs::stream_container_logs`: log stream errors at warn before exiting the forwarder loop.
Per CodeRabbit review on PR #26. * The first cargo block previously showed un-prefixed commands and buried the SQLX_OFFLINE note below — anyone copy-pasting the top block hit "DATABASE_URL must be set" errors. Hoisted the SQLX_OFFLINE requirement to a callout above the block, prefixed every Rust command in the block, and labelled the second block as the full pre-PR check matrix. * Added a one-paragraph note under Versioning pointing at the `check-version` job in release.yml — explains what it checks, what happens on mismatch, and the gotcha that `crates/core/Cargo.toml` is not yet covered by CI and must be kept in sync manually.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
CLAUDE.md (1)
162-171: Consider extending thecheck-versionjob to covercrates/core/Cargo.tomlinstead of documenting the gap.The doc correctly notes that
release.ymlchecks three files and thatcrates/core/Cargo.tomlis on manual sync, but as long as the gap exists a tagged release with a stale core-crate version will pass CI and ship. This is a ~1-line workflow change that closes the loop and lets you delete the “keep it in sync manually” caveat:# .github/workflows/release.yml (check-version job) - CORE=$(grep '^version' crates/core/Cargo.toml | head -1 | sed 's/version = "\(.*\)"/\1/') # ...add CORE to the comparison: - if [ "$CARGO" != "$VERSION" ] || [ "$CORE" != "$VERSION" ] || [ "$PKG" != "$VERSION" ] || [ "$TAURI" != "$VERSION" ]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` around lines 162 - 171, The release workflow’s check-version job in release.yml only compares src-tauri/Cargo.toml (CARGO), package.json (PKG) and src-tauri/tauri.conf.json (TAURI), leaving crates/core/Cargo.toml out of CI; update the job to extract the core crate version into a CORE variable (e.g., parse crates/core/Cargo.toml for the version) and include CORE in the existing conditional that compares CARGO, PKG and TAURI against the pushed tag (i.e., ensure the if check also validates CORE == VERSION) so the core crate version is enforced by CI alongside the other three files.crates/core/src/server/watchers.rs (1)
49-64: Log the Docker inspect error before collapsing to"error".The
Err(_)arm silently discards the underlyingbollard::Docker::inspect_containerfailure. This function is called from both the post-start verification path inlocal.rsand from the 10-minute readiness-timeout fallback at line 129, so any Docker daemon hiccup, socket disconnect, or "no such container" surfaces incubelit.logonly as a status flip to"error"with no diagnostic trail.As per coding guidelines: "All Rust files in the core crate must use the
tracingcrate for structured logging".♻️ Suggested fix
pub async fn verify_container_status( docker: &bollard::Docker, container_id: &str, ) -> &'static str { match docker.inspect_container(container_id, None).await { Ok(info) => { let running = info.state.and_then(|s| s.running).unwrap_or(false); if running { "running" } else { "error" } } - Err(_) => "error", + Err(e) => { + tracing::warn!( + container_id = %container_id, + error = %e, + "verify_container_status: docker inspect failed; reporting error" + ); + "error" + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/core/src/server/watchers.rs` around lines 49 - 64, The Err(_) arm of verify_container_status currently swallows the inspect_container error; update it to log the error with tracing::error! including the error value and the container_id for diagnostics before returning "error" (i.e., capture the Err(e) from docker.inspect_container(container_id, None).await and call tracing::error!("docker inspect failed for container {}: {:?}", container_id, e) or similar, then return "error"). Ensure the file imports tracing and use the verify_container_status function name and docker.inspect_container invocation when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@CLAUDE.md`:
- Around line 162-171: The release workflow’s check-version job in release.yml
only compares src-tauri/Cargo.toml (CARGO), package.json (PKG) and
src-tauri/tauri.conf.json (TAURI), leaving crates/core/Cargo.toml out of CI;
update the job to extract the core crate version into a CORE variable (e.g.,
parse crates/core/Cargo.toml for the version) and include CORE in the existing
conditional that compares CARGO, PKG and TAURI against the pushed tag (i.e.,
ensure the if check also validates CORE == VERSION) so the core crate version is
enforced by CI alongside the other three files.
In `@crates/core/src/server/watchers.rs`:
- Around line 49-64: The Err(_) arm of verify_container_status currently
swallows the inspect_container error; update it to log the error with
tracing::error! including the error value and the container_id for diagnostics
before returning "error" (i.e., capture the Err(e) from
docker.inspect_container(container_id, None).await and call
tracing::error!("docker inspect failed for container {}: {:?}", container_id, e)
or similar, then return "error"). Ensure the file imports tracing and use the
verify_container_status function name and docker.inspect_container invocation
when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54120cb3-59df-4115-ac10-0c01fd16383e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
CLAUDE.mdcrates/core/Cargo.tomlcrates/core/src/db/queries.rscrates/core/src/docker/images.rscrates/core/src/docker/logs.rscrates/core/src/recipes.rscrates/core/src/server/local.rscrates/core/src/server/minecraft.rscrates/core/src/server/watchers.rscrates/core/tests/db_integration.rssrc-tauri/Cargo.tomlsrc-tauri/src/commands/file_commands.rssrc-tauri/src/event_sink.rs
✅ Files skipped from review due to trivial changes (1)
- crates/core/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/core/src/server/minecraft.rs
- crates/core/tests/db_integration.rs
- crates/core/src/recipes.rs
- release.yml: extend version-check to crates/core/Cargo.toml so all four versioned files are validated before the build matrix runs. - CLAUDE.md: drop the now-stale caveat that crates/core wasn't covered by CI; update the description to read "all four files." - watchers.rs: log via tracing::warn! when verify_container_status's docker inspect call fails instead of silently mapping the error to "error" — matches the logging style added to the watchers themselves in the prior correctness pass.
This was referenced Apr 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
src-tauriinto a Cargo workspace with a newcubelit-corecrate housing all shared business logic (error types, ports, recipes, sqlx queries + migrations + offline cache, Docker orchestration, server lifecycle, RCON / backup helpers). The desktop crate is now a thin transport layer whose Tauri IPC commands are 5–15 line shims that delegate tocubelit-core. This sets up cleanly for a future CLI (v0.1.9) and HTTP/WebSocket agent (v0.2.0+) — they reusecubelit-coreand only need their ownEventSinkimpl.EventSink(transport-agnostic progress events; desktop shipsTauriEventSink) and split server orchestration intoServerRunner(narrow Docker runtime ops) andServerLifecycle(full DB-touching lifecycle).LocalServerHostimplements both for the v0.1.8 single-process desktop and is the seam future remote agents will replace.server-create-progress,server-status-changed,image-pull-progress,server-logs-{id}), and payload shape is unchanged from v0.1.7 — the frontend wasn't touched at all.fix(recipe): bump Minecraft Java image to java25). Modern Minecraft releases — including vanillaLATEST(1.21.10+ / "26.x") and any modpack built against them — ship a Java 25 bundler (class file version 69.0). The oldjava21pin producedUnsupportedClassVersionErrorbefore the server could boot, so any default install of the Minecraft Java recipe was effectively non-functional. Bumpeddefault_tagfrom"java21"→"java25".src-tauri/Cargo.toml,crates/core/Cargo.toml,package.json,tauri.conf.json, plus a CHANGELOG entry.Commit sequence (8 commits)
chore(ci): expand paths filter for upcoming workspace layoutchore: introduce Cargo workspace with empty crates/corerefactor(core): move pure types and helpers into crates/corefeat(core): move sqlx queries, migrations, and offline cache into corefeat(core): introduce EventSink trait and move docker modules to corefeat(core): move server lifecycle behind ServerRunner+ServerLifecycle traitschore: bump version to 0.1.8fix(recipe): bump Minecraft Java image to java25The desktop app builds and runs at every commit — the migration was deliberately split this way so any commit can be bisected independently.
Test plan
SQLX_OFFLINE=true cargo check --workspace— cleanSQLX_OFFLINE=true cargo build --workspace(debug) — cleanSQLX_OFFLINE=true cargo build --release --workspace— clean (~33s cold)SQLX_OFFLINE=true cargo clippy --workspace --all-targets -- -D warnings— cleanSQLX_OFFLINE=true cargo test --workspace— 33 tests passing (24 cubelit-core unit + 9 db_integration)bun run check— 265 files, 0 errors, 0 warningsbun run test— 13 tests passingcubelitv0.1.8 andcubelit-corev0.1.8lib.rsTauriEventSinkmapsCoreEventvariants onto the historical event names byte-for-bytecrates/core/.sqlx/offline cache committed and consumed bySQLX_OFFLINE=truebuildscrates/core/migrations/001_init.sqlpresent and unchangedbun run tauri dev→ create a vanilla Minecraft Java server withVERSION=LATEST→ confirm it boots past the JVM check (this is the java25 fix)ci.yml: cargo check / clippy / test,bun run check/bun run test)Notes
[workspace.package] version = ...inheritance) — that's a follow-up for v0.1.9 once the CLI lands and we want lockstep versions across multiple binaries.calcmodNoClassDefFoundError: net/minecraft/class_746) is a client-only mod incorrectly packaged for a server — not fixable from Cubelit's side.Summary by CodeRabbit
New Features
Improvements
Chores