Skip to content

Commit dc5f0b4

Browse files
committed
Type more IPC commands + sustainability for the bindings
Tightens the typed-IPC surface added in `f1e58011` and shores up the regen / sustainability story. Group A — drop `skip_serializing_if` to unblock specta Removed `#[serde(skip_serializing_if = "Option::is_none")]` (and `Vec::is_empty` variants) from `FileEntry`, `PaneState`, `MtpDeviceInfo`, `MtpStorageInfo`, `LocationInfo` (macOS / Linux / stub), `NetworkHost`, and `BundleManifest` fields. Wire format goes from "field absent" to explicit `null` / `[]`. specta's `validate_exported_command` now accepts the types, unblocking 9 commands: `get_file_range`, `get_file_at`, `get_files_at_indices`, `update_left_pane_state`, `update_right_pane_state`, `list_mtp_devices`, `connect_mtp_device`, `get_mtp_device_info`, `get_mtp_storages`, `list_mtp_directory`, `list_volumes`, `resolve_path_volume`, `list_network_hosts`, `resolve_host`, `connect_to_server`. FE side: migrated each call site from raw `invoke()` to `commands.foo(...)`, dropped the `cmdr/no-raw-tauri-invoke` opt-out comments, fixed four `=== undefined` → `!= null` patterns that the change broke (`smbConnectionState`, `recursiveSize`, `displaySize` × 2). Renamed an inner `FileEntry` in `mcp/pane_state.rs` to `PaneFileEntry` to avoid a name collision with the real `FileEntry` once both types crossed the IPC boundary. Group B (first 3) — replace `serde_json::Value` at IPC boundaries - `check_pending_crash_report` / `send_crash_report`: dropped the noop `serde_json::to_value` / `Value` wrapper. The commands now take/return `CrashReport` directly. `CrashReport` and `ActiveSettings` got `#[derive(specta::Type)]` (kept `Deserialize` since the on-disk crash file format still round-trips through it). - `record_settings_defaults`: replaced `HashMap<String, serde_json::Value>` with `HashMap<String, SettingValue>` where `SettingValue` is an untagged `Bool | Integer | String` enum. The TS surface lands as `boolean | number | string` — covers every value type the registry actually carries. `record_breadcrumb` stays on raw `invoke()` deliberately. Free-form `ctx` is the design intent for breadcrumbs; none of the typed alternatives (string-only map, JSON-encoded string, tagged-union by `kind`) is genuinely better. `prepare_error_report_preview` stays excluded for the same reason (its `BundleManifest.breadcrumbs` carries the same `Option<Value>`); a future decoupling via `BreadcrumbForManifest { ctx_json: Option<String> }` is documented in `lib/ipc/CLAUDE.md` if it ever becomes worthwhile. Sustainability wiring - `export_bindings_test` is now `#[ignore]`d so plain `cargo nextest run` doesn't silently rewrite `bindings.ts` as a test side effect. - `pnpm bindings:regen` (in `apps/desktop/package.json`) is the canonical regen command — runs the ignored test, then `oxfmt`s the output so the file lands in project format (single quotes, no semis, 2-space indent — which is what `oxfmt` enforces; my earlier confusion about "oxfmt is lenient" was wrong, the file just was already formatted). - `bindings-fresh` check (Go, `desktop-bindings-fresh.go`) snapshots the committed file, runs the regen, byte-compares, restores the snapshot. Fails on drift with a pointer to `pnpm bindings:regen`. CI catches stale bindings without touching the working tree. - `lib/ipc/CLAUDE.md` documents the named-variables call-site convention, the "adding a new Tauri command" recipe, the type-shape constraints (`skip_serializing_if`, `serde_json::Value`), the trimmed exclusion table, the version-bump procedure, and the future-work decoupling note. - `lib/tauri-commands/CLAUDE.md` § Key patterns now reflects that wrappers delegate to the typed bindings; raw `invoke` is the exception (with the documented opt-out). - AGENTS.md "Type-safe IPC" entry links to the regen flow and the named-variables rule. - `knip.json` ignores `oxfmt` as an unlisted binary (used by `bindings:regen`). Numbers - 4 commands remain excluded (down from 14): `record_breadcrumb`, `prepare_error_report_preview`, `store_font_metrics`, `stream_folder_suggestions`/`cancel_folder_suggestions`. Each has a documented reason and conversion plan. - All 45 checks pass. 1601 Rust + 1716 Svelte + 124 API + 67 Astro e2e + 28 Rust integration tests pass. `bindings-fresh` confirms `bindings.ts` is in sync.
1 parent f1e5801 commit dc5f0b4

39 files changed

Lines changed: 3088 additions & 2051 deletions

AGENTS.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,13 @@ resilience, and common pitfalls.
219219
bindings into `apps/desktop/src/lib/ipc/`; call them as `commands.commandName(args)` instead.
220220
- **Enforced by**: `cmdr/no-raw-tauri-invoke` (ESLint rule). Bypassed only inside `lib/ipc/` (the bindings),
221221
`routes/debug/` (dev-only debug panels), and test files.
222+
- **Regenerate** with `cd apps/desktop && pnpm bindings:regen` after any change to a `#[tauri::command]` surface or a
223+
Type-derived DTO. CI's `bindings-fresh` check fails if the committed `bindings.ts` is stale.
224+
- **At call sites, prefer named locals over inline primitives.** `commands.renameFile(from, to, force, volumeId)` is
225+
fine; `commands.foo(true, null, 5)` isn't — extract `const force = true; const volumeId = null; const retries = 5`
226+
first. This is the price specta charges for type safety.
227+
- For the rules around adding new commands, type shape constraints (`skip_serializing_if`, `serde_json::Value`), and
228+
the current exclusion list, read [`apps/desktop/src/lib/ipc/CLAUDE.md`](apps/desktop/src/lib/ipc/CLAUDE.md).
222229

223230
## Workflow
224231

apps/desktop/knip.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"$schema": "https://unpkg.com/knip@5/schema.json",
3-
"ignoreBinaries": ["only-allow", "rustc"],
3+
"ignoreBinaries": ["only-allow", "rustc", "oxfmt"],
44
"ignore": ["src/lib/tauri-commands/**", "src/lib/ipc/bindings.ts"],
55
"ignoreUnresolved": ["~icons/.+"],
66
"ignoreDependencies": [

apps/desktop/package.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,8 @@
2727
"test:e2e:playwright": "npx playwright test --config test/e2e-playwright/playwright.config.ts --project tauri",
2828
"test:e2e:playwright:build": "node scripts/tauri-wrapper.js build --no-bundle --target $(rustc -vV | grep host | cut -d' ' -f2) -- --features playwright-e2e,virtual-mtp",
2929
"knip": "knip",
30-
"check:type-drift": "tsx scripts/check-type-drift.ts"
30+
"check:type-drift": "tsx scripts/check-type-drift.ts",
31+
"bindings:regen": "cd src-tauri && cargo nextest run --no-fail-fast --run-ignored=ignored-only ipc::tests::export_bindings_test && cd .. && pnpm exec oxfmt src/lib/ipc/bindings.ts"
3132
},
3233
"license": "SEE LICENSE IN LICENSE",
3334
"dependencies": {

apps/desktop/src-tauri/src/commands/crash_reporter.rs

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
//! Thin wrappers for crash file detection, dismissal, and sending.
44
55
use crate::config;
6-
use crate::crash_reporter;
6+
use crate::crash_reporter::{self, CrashReport};
77

88
/// Server URL for crash report ingestion.
99
#[cfg(debug_assertions)]
@@ -13,12 +13,11 @@ const CRASH_REPORT_URL: &str = "http://localhost:8787/crash-report";
1313
const CRASH_REPORT_URL: &str = "https://api.getcmdr.com/crash-report";
1414

1515
/// Checks for a pending crash report from a previous session.
16-
/// Returns the report as a JSON value, or `null` if none exists.
16+
/// Returns the report, or `null` if none exists.
1717
#[tauri::command]
1818
#[specta::specta]
19-
pub fn check_pending_crash_report(app: tauri::AppHandle) -> Option<serde_json::Value> {
20-
let report = crash_reporter::take_pending_crash_report(&app)?;
21-
serde_json::to_value(report).ok()
19+
pub fn check_pending_crash_report(app: tauri::AppHandle) -> Option<CrashReport> {
20+
crash_reporter::take_pending_crash_report(&app)
2221
}
2322

2423
/// Deletes the crash report file without sending it.
@@ -36,7 +35,7 @@ pub fn dismiss_crash_report(app: tauri::AppHandle) {
3635
/// Skipped in dev mode and CI to avoid polluting production data.
3736
#[tauri::command]
3837
#[specta::specta]
39-
pub async fn send_crash_report(app: tauri::AppHandle, report: serde_json::Value) -> Result<(), String> {
38+
pub async fn send_crash_report(app: tauri::AppHandle, report: CrashReport) -> Result<(), String> {
4039
let should_skip = cfg!(debug_assertions) || std::env::var("CI").is_ok();
4140

4241
if !should_skip {

apps/desktop/src-tauri/src/commands/error_reporter.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,9 @@
33
//! Two preview/send commands plus a debug-only save-to-disk command. Business logic lives
44
//! in `crate::error_reporter` — these wrappers just shape inputs/outputs for the IPC layer.
55
6-
use crate::error_reporter::{self, BundleKind, BundleManifest, BundleScope, FLOW_A_BUNDLE_CAP_MB};
6+
use crate::error_reporter::{
7+
self, BundleKind, BundleManifest, BundleScope, FLOW_A_BUNDLE_CAP_MB, settings_defaults::SettingValue,
8+
};
79
use serde::Serialize;
810
use std::collections::HashMap;
911

@@ -28,7 +30,7 @@ const MAX_USER_NOTE_CHARS: usize = 100_000;
2830
/// is missing or doesn't include a given key.
2931
#[tauri::command]
3032
#[specta::specta]
31-
pub fn record_settings_defaults(defaults: HashMap<String, serde_json::Value>) {
33+
pub fn record_settings_defaults(defaults: HashMap<String, SettingValue>) {
3234
error_reporter::settings_defaults::record(defaults);
3335
}
3436

apps/desktop/src-tauri/src/crash_reporter/mod.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ static APP_START_TIME: OnceLock<Instant> = OnceLock::new();
2727
static CACHED_SETTINGS: OnceLock<ActiveSettings> = OnceLock::new();
2828

2929
/// Active settings snapshot cached at startup for inclusion in crash reports.
30-
#[derive(Debug, Clone, Default, Serialize, Deserialize)]
30+
#[derive(Debug, Clone, Default, Serialize, Deserialize, specta::Type)]
3131
#[serde(rename_all = "camelCase")]
3232
pub struct ActiveSettings {
3333
pub indexing_enabled: Option<bool>,
@@ -37,7 +37,7 @@ pub struct ActiveSettings {
3737
}
3838

3939
/// The crash report written to disk (JSON).
40-
#[derive(Debug, Serialize, Deserialize)]
40+
#[derive(Debug, Serialize, Deserialize, specta::Type)]
4141
#[serde(rename_all = "camelCase")]
4242
pub struct CrashReport {
4343
pub version: u32,

apps/desktop/src-tauri/src/error_reporter/mod.rs

Lines changed: 36 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -288,9 +288,7 @@ pub struct BundleManifest {
288288
/// first. The last entry of `kind: "command"` is the most recent user-driven UI
289289
/// command. Empty when nothing was recorded (e.g. very early failures, tests).
290290
/// See `breadcrumbs.rs` for the buffer semantics.
291-
#[serde(skip_serializing_if = "Vec::is_empty", default)]
292291
pub breadcrumbs: Vec<breadcrumbs::Breadcrumb>,
293-
#[serde(skip_serializing_if = "Option::is_none")]
294292
pub user_note: Option<String>,
295293
pub generated_at: String,
296294
}
@@ -1182,39 +1180,67 @@ pub mod log_level_overrides {
11821180
/// Reads are O(1) hash lookups; the mutex is uncontended in practice (one writer at
11831181
/// FE init, then read-only).
11841182
pub mod settings_defaults {
1185-
use serde_json::Value;
1183+
use serde::{Deserialize, Serialize};
11861184
use std::collections::HashMap;
11871185
use std::sync::Mutex;
11881186

1189-
static DEFAULTS: Mutex<Option<HashMap<String, Value>>> = Mutex::new(None);
1187+
/// Settings registry default values pushed from FE. The wire format matches JSON
1188+
/// primitives via `#[serde(untagged)]` — TS sees `boolean | number | string`.
1189+
/// Extend with more variants only when settings of the new shape appear; any field
1190+
/// shape outside this set means the value can't fit in `lookup_*` helpers anyway.
1191+
#[derive(Debug, Clone, Serialize, Deserialize, specta::Type)]
1192+
#[serde(untagged)]
1193+
pub enum SettingValue {
1194+
Bool(bool),
1195+
Integer(i64),
1196+
String(String),
1197+
}
1198+
1199+
static DEFAULTS: Mutex<Option<HashMap<String, SettingValue>>> = Mutex::new(None);
11901200

11911201
/// Replace the stored defaults map. Called from the `record_settings_defaults`
11921202
/// Tauri command. A `None` entry from a buggy FE caller is treated as "no
11931203
/// default available" — `lookup_*` falls through to the hardcoded fallback.
1194-
pub fn record(map: HashMap<String, Value>) {
1204+
pub fn record(map: HashMap<String, SettingValue>) {
11951205
if let Ok(mut guard) = DEFAULTS.lock() {
11961206
*guard = Some(map);
11971207
}
11981208
}
11991209

1200-
fn get(key: &str) -> Option<Value> {
1210+
fn get(key: &str) -> Option<SettingValue> {
12011211
DEFAULTS.lock().ok()?.as_ref()?.get(key).cloned()
12021212
}
12031213

12041214
pub(super) fn lookup_bool(key: &str) -> Option<bool> {
1205-
get(key)?.as_bool()
1215+
if let SettingValue::Bool(b) = get(key)? {
1216+
Some(b)
1217+
} else {
1218+
None
1219+
}
12061220
}
12071221

12081222
pub(super) fn lookup_string(key: &str) -> Option<String> {
1209-
Some(get(key)?.as_str()?.to_string())
1223+
if let SettingValue::String(s) = get(key)? {
1224+
Some(s)
1225+
} else {
1226+
None
1227+
}
12101228
}
12111229

12121230
pub(super) fn lookup_u16(key: &str) -> Option<u16> {
1213-
u16::try_from(get(key)?.as_u64()?).ok()
1231+
if let SettingValue::Integer(n) = get(key)? {
1232+
u16::try_from(n).ok()
1233+
} else {
1234+
None
1235+
}
12141236
}
12151237

12161238
pub(super) fn lookup_u64(key: &str) -> Option<u64> {
1217-
get(key)?.as_u64()
1239+
if let SettingValue::Integer(n) = get(key)? {
1240+
u64::try_from(n).ok()
1241+
} else {
1242+
None
1243+
}
12181244
}
12191245

12201246
#[cfg(test)]

apps/desktop/src-tauri/src/error_reporter/tests.rs

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -500,9 +500,8 @@ fn set_mtime_to_age(path: &Path, age: Duration) {
500500
/// fields are `None`.
501501
mod settings_defaults_tests {
502502
use super::*;
503-
use crate::error_reporter::settings_defaults;
503+
use crate::error_reporter::settings_defaults::{self, SettingValue};
504504
use crate::settings::loader::Settings;
505-
use serde_json::json;
506505
use std::collections::HashMap;
507506
use std::sync::Mutex;
508507

@@ -544,9 +543,9 @@ mod settings_defaults_tests {
544543
// FE registry says indexing is OFF by default (hypothetical — production says
545544
// true). If `from_settings` ever drifts back to the hardcoded fallback, this
546545
// test catches it.
547-
map.insert("indexing.enabled".to_string(), json!(false));
548-
map.insert("developer.mcpPort".to_string(), json!(12345));
549-
map.insert("ai.provider".to_string(), json!("cloud"));
546+
map.insert("indexing.enabled".to_string(), SettingValue::Bool(false));
547+
map.insert("developer.mcpPort".to_string(), SettingValue::Integer(12345));
548+
map.insert("ai.provider".to_string(), SettingValue::String("cloud".to_string()));
550549
settings_defaults::record(map);
551550

552551
// Field with no user override → FE default applies.
@@ -576,8 +575,12 @@ mod settings_defaults_tests {
576575
settings_defaults::reset_for_test();
577576

578577
let mut map = HashMap::new();
579-
map.insert("indexing.enabled".to_string(), json!("not a bool"));
580-
map.insert("developer.mcpPort".to_string(), json!(true));
578+
// Wrong types for these fields: String instead of Bool, Bool instead of Integer.
579+
map.insert(
580+
"indexing.enabled".to_string(),
581+
SettingValue::String("not a bool".to_string()),
582+
);
583+
map.insert("developer.mcpPort".to_string(), SettingValue::Bool(true));
581584
settings_defaults::record(map);
582585

583586
let resolved = ResolvedSettings::from_settings(&Settings::default());

apps/desktop/src-tauri/src/file_system/listing/metadata.rs

Lines changed: 2 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -68,8 +68,8 @@ pub(crate) fn get_icon_id(is_dir: bool, is_symlink: bool, name: &str) -> String
6868
/// Represents a file or directory entry with extended metadata.
6969
///
7070
/// Only serialized (Rust → frontend); never sent from the frontend, so no `Deserialize`.
71-
/// The `Deserialize`-less derive prevents specta from splitting the type on
72-
/// `skip_serializing_if` fields, which would fail `validate_exported_command`.
71+
/// Fields that were previously omitted when `None`/empty now serialize as explicit `null`
72+
/// so specta's `validate_exported_command` accepts the type in Unified mode.
7373
#[derive(Debug, Clone, Serialize, specta::Type)]
7474
#[serde(rename_all = "camelCase")]
7575
pub struct FileEntry {
@@ -79,8 +79,6 @@ pub struct FileEntry {
7979
pub is_symlink: bool,
8080
pub size: Option<u64>,
8181
/// Physical size on disk in bytes (st_blocks * 512 on Unix, same as size on other platforms)
82-
#[serde(default, skip_serializing_if = "Option::is_none")]
83-
#[specta(optional)]
8482
pub physical_size: Option<u64>,
8583
pub modified_at: Option<u64>,
8684
pub created_at: Option<u64>,
@@ -96,35 +94,23 @@ pub struct FileEntry {
9694
/// Always true for legacy list_directory(), false for list_directory_core()
9795
pub extended_metadata_loaded: bool,
9896
/// Recursive size in bytes (from drive index, None if not indexed)
99-
#[serde(default, skip_serializing_if = "Option::is_none")]
100-
#[specta(optional)]
10197
pub recursive_size: Option<u64>,
10298
/// Recursive physical size on disk in bytes (from drive index, None if not indexed)
103-
#[serde(default, skip_serializing_if = "Option::is_none")]
104-
#[specta(optional)]
10599
pub recursive_physical_size: Option<u64>,
106100
/// Recursive file count (from drive index, None if not indexed)
107-
#[serde(default, skip_serializing_if = "Option::is_none")]
108-
#[specta(optional)]
109101
pub recursive_file_count: Option<u64>,
110102
/// Recursive dir count (from drive index, None if not indexed)
111-
#[serde(default, skip_serializing_if = "Option::is_none")]
112-
#[specta(optional)]
113103
pub recursive_dir_count: Option<u64>,
114104
/// True when the subtree contains symlinks (whose content is omitted from the
115105
/// recursive size). Drives the "size omits symlinked content" hint in the UI.
116106
/// `None` when the directory isn't indexed yet.
117-
#[serde(default, skip_serializing_if = "Option::is_none")]
118-
#[specta(optional)]
119107
pub recursive_has_symlinks: Option<bool>,
120108
/// When set on a virtual entry, the frontend navigates to this path
121109
/// instead of treating the entry as a normal directory listing.
122110
/// Inert until M3 wires it for `worktrees/` and `submodules/`. Lives on
123111
/// the schema from M1 so M3 doesn't have to ripple a change through every
124112
/// consumer (frontend list views, MCP `cmdr://state`, drag-drop, copy
125113
/// preview, Brief/Full renderers).
126-
#[serde(default, skip_serializing_if = "Option::is_none")]
127-
#[specta(optional)]
128114
pub redirect_to_path: Option<String>,
129115
/// Loose Size-column override for virtual git entries: rendered verbatim
130116
/// in the Full mode Size column instead of formatted bytes from `size`.
@@ -133,14 +119,10 @@ pub struct FileEntry {
133119
/// branches, files-changed for commits, item count for category roots).
134120
/// Cross-category Size sorting is meaningless and that's an honest
135121
/// tradeoff — each cell is self-explaining via tooltip + aria-label.
136-
#[serde(default, skip_serializing_if = "Option::is_none")]
137-
#[specta(optional)]
138122
pub display_size: Option<String>,
139123
/// Optional rich tooltip string for the Size cell, used when
140124
/// `display_size` is set. Example: "12 commits ahead, 3 commits behind
141125
/// `origin/main`". Doubles as the aria-label for screen readers.
142-
#[serde(default, skip_serializing_if = "Option::is_none")]
143-
#[specta(optional)]
144126
pub display_size_tooltip: Option<String>,
145127
}
146128

0 commit comments

Comments
 (0)