Replace Slice by Observable, simplify and clean up the Configurable mechanism#391
Merged
Conversation
New src/observable.rs introduces a typed state holder that fans out
changes to a list of subscriber closures. Subscribers run synchronously
from the write guard's Drop, the same point where Slice<T> emits and
flushes today.
Two recurring patterns get convenience builders:
* .emit_to(&app, "event_name") — Tauri event emission
* .persist_to(backend) — JSON-serialize and flush via the
existing Persistence trait
Anything more exotic (debouncing, breadcrumbs, custom backends) is just
.subscribe(move |value| ...). Persistence stays as a trait so future
backends can layer debouncing/batching without touching the core type.
No callers in this commit; existing Slice<T> users keep working. The new
module carries #![allow(dead_code)] until callers migrate in C2-C5.
evolve::config::load_slice becomes load_observable; persistence is now attached via the .persist_to() subscriber, and the change-event name flows through .emit_to() rather than being a struct field of the slice. The onboarding fallback that used VolatileJson is now simply: if no storage path is available, build the observable without a persistence subscriber. That deletes VolatileJson (it had no other users) and removes one layer of indirection. Configurable derive updated to look for `Observable<#name>` in tauri::State and call `write_sync()` without an emitter argument. EvolutionLimits is currently the only Configurable in the codebase, so this change is isolated. Other callers updated to use the new type: - storage/store.rs (4 get/set helpers) - commands/settings_io.rs (export + import) - main.rs (two manage() call sites)
evolve_state::load_slice becomes load_observable. The change-event name flows through .emit_to() and persistence is attached via .persist_to(). get() and set() switch from try_state::<Slice<EvolveState>> to try_state::<Observable<EvolveState>>; write_sync() drops its emitter argument. The "no managed observable" fallback path (used during early startup before app.manage) keeps its bespoke emit + flush sequence. main.rs updated for both manage() call sites.
state::preferences::load_global_slice becomes load_global_observable. Event name flows through .emit_to(); persistence is attached via .persist_to(). main.rs updated for both manage() call sites; commands/settings_io.rs switches the export and import paths to try_state::<Observable<GlobalPreferences>>(), and the write_sync call drops its emitter argument. This is the last Slice<T> caller. The type itself, SliceWriteGuard, and the SliceEventEmitter trait now have no users; state/slice/mod.rs gets #![allow(dead_code)] as a one-commit shim. The next commit (C6) deletes the dead code and moves persistence + json_io under src/observable/.
After the three caller migrations, Slice<T> has no users left. This
commit:
- Deletes Slice, SliceWriteGuard, and the SliceEventEmitter trait from
state/slice/mod.rs. The three slice-level tests are absorbed by
observable's own tests; persistence + registry tests move with their
respective code.
- Moves apps/native/src-tauri/src/observable.rs to
src/observable/mod.rs and pulls persistence.rs + json_io.rs under it.
The Persistence trait + AppDataJson + RepoScopedJson are re-exported
at crate::observable::*, so callers say
`use crate::observable::{AppDataJson, Persistence, Observable}`.
- Shrinks state/slice/mod.rs to a thin re-export of registry.rs. The
module stays as the home for SliceRegistry until B1 retires it.
- Updates state/mod.rs prose to describe the new shape (observables, not
slices).
Drops the runtime SliceRegistry / RegisteredSliceConfig pair in favor of
compile-time registration via the `inventory` crate, addressing B1 from
docs/2026-06-03-pr-review-followups.md.
The derive now emits one `inventory::submit!{ ConfigurableMeta {...} }`
per #[derive(Configurable)] struct, alongside the existing Wry-specialized
shim functions. `ConfigurableMeta` lives in the `configurable` crate and
carries the same triple as the old RegisteredSliceConfig (name +
schema_fn + set_field_fn). The `inventory` crate is re-exported from
`configurable::inventory` so the derive output never needs the consuming
crate to add it as a direct dep.
`commands/dev_configs.rs` walks `inventory::iter::<ConfigurableMeta>()`
instead of looking up `tauri::State<SliceRegistry>`; a new test confirms
the link-time submit actually lands ("EvolutionLimits is registered via
inventory") so a future toolchain regression on linker sections is
caught before dev settings silently goes empty.
Deleted:
- apps/native/src-tauri/src/state/slice/ (whole directory)
- evolve::config::register_slice_config
- the app.manage(SliceRegistry::default()) line in main.rs
- the register_slice_config(...) call in the Tauri setup hook
- state/mod.rs stops re-exporting the `slice` submodule
main.rs no longer references EvolutionLimits or any other Configurable
struct directly — registration happens entirely through the derive.
Addresses B3 from docs/2026-06-03-pr-review-followups.md. The schema half
is now pure static metadata and no longer requires an AppHandle; the
dynamic half (current store-backed values) is fetched separately and the
two are joined at the IPC boundary.
Type changes in the configurable crate:
- ConfigField is gone. Split into:
* ConfigFieldSchema { key, label, help, ty, default } — static
* ConfigFieldValue { key, current } — dynamic
- ConfigurableSchema.fields: Vec<ConfigFieldSchema> (no `current` field)
- New ConfigurableSnapshot { schema, values } is what dev_configs_list
returns to the frontend.
- ConfigurableMeta gains a `load_value_fn` pointer; `schema_fn` drops
its AppHandle argument and now returns ConfigurableSchema directly
(no Result, no app).
Derive macro:
- schema() generated without an `app` parameter; same value every call.
- Adds __configurable_load_value_wry that calls Self::load(app) and
serializes to serde_json::Value for the IPC join.
commands/dev_configs.rs:
- dev_configs_list returns Vec<ConfigurableSnapshot>.
- snapshot_for() joins the static schema with current values by key.
specta_gen_ts.rs registers the new types; ipc/types.ts regenerated.
Frontend:
- tauriAPI.devConfigs.list() returns ConfigurableSnapshot[].
- AutoConfigField now takes `field: ConfigFieldSchema` and a separate
`current: JsonValue` prop, instead of pulling `current` off the field.
- Both auto-tuning-section.tsx and tuning-tab.tsx build a per-snapshot
valuesByKey map and pass each field's current value in.
- Storybook stories updated to the new snapshot shape.
Acceptance per the followup doc:
- `EvolutionLimits::schema()` is callable without an AppHandle.
- Same value every call.
- IPC payload joins schema and values at the command boundary.
PR #330 removed the queue-summarizer pipeline (per-hunk grouping replaced by a whole-diff pass), but left the queued_summaries table in 01-initial/up.sql and its Diesel declaration in tables.rs. Every fresh nixmac database has been initializing a table that nothing ever touches. This commit: - Adds 03-drop-queued-summaries migration that drops the table and the idx_queued_summaries_status index. The original CREATE in 01-initial stays untouched so existing user_version=1 databases still apply the full migration history when they upgrade. - Removes the diesel::table! declaration and the allow_tables_to_appear_in_same_query! entry from db/tables.rs. - Deletes the QueuedSummary struct from sqlite_types.rs (it was already marked #[allow(dead_code)] and had no remaining importers). - Adds a migration_03_drops_queued_summaries_table test in db/mod.rs that asserts a freshly initialized database does not contain the table. - Drops the stale queue_summarizer.rs line from src/README.md.
…peline The src-tauri/README.md still listed the per-hunk grouping module set that PR #330 deleted (assignments.rs, simplify_grouped.rs, model_output_types.rs, fresh_changeset/evolved_changeset pipelines). The db/ section likewise listed two deleted store_*_changeset.rs files. Rewrites the summarize/ bullet list to match the current tree (mod, find_existing, group_existing, model_calls, build_prompt, token_budgets, sumlog) and the pipelines/ entries (whole_diff, history, commit_message). Adds a one-line note explaining the whole-diff direction so a future reader doesn't have to dig through git log. In the db/ section, replaces store_new_changeset / store_evolved_changeset with the surviving store_whole_diff_changeset, and adds pool.rs + tables.rs which already existed but were missing from the listing.
Addresses B4 from docs/2026-06-03-pr-review-followups.md. Replaces the
per-field set_field(struct_name, key, value) dispatch with a whole-struct
set(struct_name, value) that lets Serde validate every field in one pass
on the way into a typed Self.
Backend:
- ConfigurableMeta.set_field_fn -> set_fn (now takes one Value, no key).
- Derive emits `set<R: Runtime>(app, value)` instead of `set_field(app,
key, value)`. Body: deserialize the JSON payload into Self via
serde_json::from_value, then replace *observable.write_sync() with
the new value. One Serde error per failure, not a per-field match arm.
- Derive drops the `__configurable_set_field_wry` shim in favor of
`__configurable_set_wry`, with the matching inventory::submit! update.
- fields.rs::FieldCode drops the set_field_arm fragment, and
GeneratedFields drops the set_field_arms vec; generate_fields no longer
needs the struct name string.
- dev_config_set IPC command drops its `key` parameter; signature is now
(app, struct_name, value).
Frontend:
- tauriAPI.devConfigs.set(structName, value) — payload is the whole
struct (every field), not a partial update.
- AutoConfigField swaps the direct tauriAPI call for an `onCommit(key,
value)` callback the parent supplies. Parent owns the snapshot; on
each field commit it overlays the new value onto the snapshot's
existing values, POSTs the full struct, and updates its local state
so the next commit reads back the freshly persisted value.
- auto-tuning-section.tsx and tuning-tab.tsx both grow a small
commitField helper that does the overlay + POST + return-next-snapshot.
- Storybook stories pass `onCommit={async () => undefined}` for static
rendering.
Tradeoff: whole-struct writes clobber any concurrent backend-side edits
to other fields. Acceptable for the single-user dev settings panel
(documented in §2.4 of the followup doc); if a multi-actor config surface
ever shows up, this assumption would need revisiting.
ConfigurableMeta had `load_value_fn` next to `set_fn` and `schema_fn` — the `_value` suffix was meant to flag "returns JSON, not Self", but every field on ConfigurableMeta is a fn pointer that operates on serde_json::Value at the boundary, so the suffix wasn't carrying information. Renames `load_value_fn` to `load_fn` in the ConfigurableMeta struct, the derive's `__configurable_load_value_wry` shim to `__configurable_load_wry`, and the one read site in dev_configs.rs. The typed concrete method `EvolutionLimits::load(app) -> Self` was already named symmetrically with `set(app, Value)`; this aligns the type-erased registry shim with the same convention.
dev_configs_list (returning the joined ConfigurableSnapshot) bundled two
things that have different cacheability: the static schema (same value
every call) and the dynamic current values (change on every set).
Splitting the IPC into two commands mirrors the type-level split B3
already established and lets the frontend cache schemas independently
of value refreshes.
Backend:
- dev_configs_schemas -> Vec<ConfigurableSchema>: static metadata only.
- dev_configs_values -> HashMap<String, serde_json::Value>: current
state of every Configurable, keyed by struct name. Each value is the
full struct as a JSON object — same shape load_fn already returns.
- Deleted dev_configs_list and the snapshot-construction helper.
- main.rs registers the two new commands in place of the old one.
Type cleanup in the configurable crate:
- Deleted ConfigurableSnapshot (frontend joins schemas + values itself).
- Deleted ConfigFieldValue (values come back as struct-shaped JSON
objects, so a separate {key, current} wrapper carries no information).
- ConfigFieldSchema, ConfigurableSchema, FieldType, EnumVariant survive
and are the only configurable-domain types on the IPC boundary.
- specta_gen_ts.rs updated; ipc/types.ts regenerated.
Frontend:
- tauriAPI.devConfigs gains schemas() and values(); list() removed.
- AutoTuningSection and TuningTab keep two pieces of state: schemas
(ConfigurableSchema[]) and values (Record<string, JsonValue>). They
Promise.all both on mount; on commit they overlay the new field on
the struct's existing values, POST the whole struct, and update only
the values map. Schemas don't need refreshing.
- readStructValues helper extracts the struct's value map (handling the
"not yet loaded" case as an empty object).
- Storybook mocks updated to expose schemas() + values() + set()
instead of list() + set().
Net concept count in the configurable crate: down from 4 wrapper types
(ConfigFieldSchema, ConfigFieldValue, ConfigurableSchema,
ConfigurableSnapshot) to 2 (ConfigFieldSchema, ConfigurableSchema).
Addresses B6 from docs/2026-06-03-pr-review-followups.md. The
generate_evolution loop read configurable limits with:
EvolutionLimits::load(app)
.inspect_err(|e| warn!("EvolutionLimits::load failed ({e}); using defaults"))
.unwrap_or_default()
This was dead-error-handling AND a silent fallback. The derive-generated
`load` never returns Err today (it either reads the managed observable
or synthesizes field defaults), but the `.unwrap_or_default()` would
mask a real misconfiguration if a future refactor missed
`app.manage(load_observable(handle)?)` in main.rs's setup hook. The
agent would then run with EvolutionLimits::default() instead of
panicking visibly.
Replaces with a direct `app.state::<Observable<EvolutionLimits>>()
.read_sync().clone()`. `app.state` panics naturally on missing managed
state, which is the correct behavior for a startup-time misconfig.
Investigation notes:
- generate_evolution has exactly one caller (evolve::lifecycle), which
is reached only after Tauri setup completes. The observable is always
managed by that point in production.
- No tests reach generate_evolution; no Tauri mock infrastructure
anywhere in the codebase. The fallback was purely defensive against
a hypothetical setup bug.
- The derive's `load` keeps its existing contract (Result<Self> with
the silent-fallback path) — not changing the derive surface as part
of B6 since the call-site fix is sufficient. Future Configurables
that want loud-on-missing should call `app.state` directly the same
way.
Also refreshes the stale comment on `impl Default for EvolutionLimits`:
it was claiming Default catches "load failures during onboarding," but
ConfiguredRepoScopedJson handles the "no config_dir yet" case at the
persistence layer (returns None, never errors). Default is actually
load-bearing for two reasons: `preferences::load_or_default` falls back
to it when the JSON file is absent, and `#[serde(default)]` uses it
when a whole-struct payload arrives with missing fields.
Includes minor treefmt fixups in observable/{mod,persistence}.rs and
commands/{dev_configs,settings_io}.rs that the linter applied during
this commit's pre-commit pass — import sort, trailing-blank-line trim,
and an assert_eq! line break. No behavior change.
Juanpe Bolívar (arximboldi)
requested review from
cooper (czxtm) and
Scott McMaster (scottmcmaster)
Contributor
🎨 Storybook previewUpdated for 7de4169 ❌ Failed snapshots (1)These stories' HTML snapshots changed. Current renderings (run Widget/Steps/SetupStep › Default Config Required |
Contributor
📋 PR Overview
🔬 Coverage
|
Juanpe Bolívar (arximboldi)
force-pushed
the
jp/refactor-the-refactor
branch
from
71a001a to
7de4169
Compare
cooper (czxtm)
approved these changes
Jun 12, 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
2 participants
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
Follows up on #317, taking care of some of my comments in PR's #228, #244–#255.
The main things happening here are:
SlicebecomesObservablewith a more "single responsibility" principle API (call callbackas on change) and things like auto-persistence and auto-emit-to-JS added on top.SliceRegistrygoes away, theConfigurablebuilds a static metadata registry (usinginventory!) under the hood; less code and less error prone.ConfigurableAPI various cleanups: separating schema from values (static from dynamic parts), symmetric setter/getter, etc.Some other minor changes or fixes (see commit by commit)
Test Plan
Docs