diff --git a/REFACTOR.md b/REFACTOR.md new file mode 100644 index 0000000..576f927 --- /dev/null +++ b/REFACTOR.md @@ -0,0 +1,159 @@ +# Refactoring review + +This review is based on a direct reading of the Rust source. It focuses on +idiomatic Rust, duplicate code, and abstractions that do not clearly earn their +complexity. Any implementation work must preserve DS4 behavior, especially in +model execution, token processing, context accounting, and KV-cache handling. + +## Overall assessment + +The project is generally deliberate, concrete Rust. Error propagation is +usually explicit, ownership is understandable, and the code avoids broad trait +or generic frameworks that would obscure DS4 behavior. The main maintainability +issue is concentration: several files and state machines are large enough that +duplication and invariants are becoming harder to see. + +## Recommended work + +### 1. Add semantic constructors for database messages + +Priority: medium + +`NewMessage` literals are repeated throughout `src/database.rs`, particularly +in `start_chat_turn`, `continue_tool_turn`, and `record_compaction`. System, +user, tool-result, assistant, and compaction rows repeatedly specify every role +flag and optional field. + +Add small role-specific constructors such as `system`, `user`, `tool_result`, +and `assistant`. This would centralize role invariants and reduce the chance of +missing a field when the schema changes. Avoid a generic builder: the semantic +constructors should make each transaction more explicit, not less. + +### 2. Split `App::update` into domain dispatch methods + +Priority: medium + +`App::update` in `src/app.rs` handles window lifecycle, preferences, projects, +generation, A2UI, Git, model downloads, cache management, and native +integration. Domain-oriented `impl App` modules already exist, but most message +dispatch remains in one very large match. + +Keep the single public Iced update entry point and delegate coherent message +families to private methods in the existing domain modules. The long preference +section is a good first extraction. Avoid splitting `App` into controller +objects unless there is a stronger ownership reason; doing so would likely add +borrow complexity without improving behavior. + +### 3. Consolidate runtime command submission + +Priority: medium + +`GenerationService::generate`, `compact`, and `measure_context` duplicate +cancellation-token creation, event-channel creation, command construction, +sending, and `ActiveGeneration` construction. + +Their metrics behavior is also inconsistent: + +- `generate` records queued work and records a rejected request if sending + fails. +- `measure_context` records queued work but does not record rejection if + sending fails. +- `compact` records neither queued work nor rejection. + +Introduce a private submission helper with an explicit tracking policy. This +would remove plumbing duplication while making intentional metrics differences +visible. + +### 4. Remove the parallel `ResponseKind` enum + +Priority: low + +`Operation` is converted into `ResponseKind` only to choose the matching error +event. This represents the same state twice and creates a small drift risk. + +Prefer an `Operation::error_event` method or one direct match at the error site. + +### 5. Clarify checkpoint and request-source modeling + +Priority: low + +`CheckpointTarget` currently combines storage location, request origin, and +lifecycle. `OneShot(PathBuf)` is also constructed with an empty path for context +measurement even though measurement does not use a checkpoint. + +Consider separating work source from checkpoint policy, or representing +measurement as a command that has no checkpoint. This area affects cache and +context behavior, so any change needs explicit DS4 parity coverage before it is +implemented. + +### 6. Concentrate repeated Metal FFI invariants + +Priority: medium, high risk + +`src/engine/metal.rs` contains many direct unsafe native calls throughout +high-level execution logic. Some operations are already wrapped by Rust types, +but many call sites still uphold buffer, offset, event, and lifecycle invariants +locally. + +Incrementally add small safe wrappers around proven repeated operations. Do not +introduce a generalized GPU framework or reorganize execution merely to reduce +the number of unsafe blocks. Changes here must be verified against DS4. + +### 7. Review argument-count suppressions selectively + +Priority: low to medium + +There are numerous `too_many_arguments` suppressions in the Metal executors, +runtime, metrics, response generation, and A2UI rendering. Explicit tensor or +kernel arguments often mirror the operation clearly and should remain that way. + +Runtime and metrics calls are better candidates for small context structs when +their arguments describe one coherent invocation or observation. Do not create +one-use parameter objects solely to satisfy Clippy. + +### 8. Split A2UI only at natural pure boundaries + +Priority: low + +`src/a2ui.rs` combines store mutation, streaming parsing, catalog validation, +function evaluation, formatting, JSON-pointer mutation, and tests. The A2UI view +module is similarly large. + +Pure expression evaluation, formatting, and catalog validation are reasonable +module boundaries. Avoid arbitrary per-component modules, which would scatter +the interpreter without reducing conceptual complexity. + +## No refactoring recommended + +### Generated hotlist data + +`src/engine/metal/hotlist.rs` is mechanically generated and should not be +treated as hand-written duplication. Keeping deterministic embedded data with +its import script is preferable to adding runtime machinery merely to reduce +source size. + +### Preference-to-engine conversion + +The preference types and effective engine-setting types in `src/settings.rs` +serve a useful normalization boundary. Optional user inputs are validated and +converted into concrete DS4-compatible runtime values. This is useful layering, +not unnecessary abstraction. + +### Concrete model descriptions + +The model `Shape` constants in `src/engine.rs` reuse common values with struct +update syntax while keeping model differences explicit. This is concise and +appropriate for behavior-sensitive model definitions. + +## Suggested order + +1. Add semantic `NewMessage` constructors. +2. Consolidate runtime command submission and document metrics policy. +3. Break `App::update` into private domain dispatch methods. +4. Remove `ResponseKind`. +5. Tighten repeated Metal FFI operations incrementally. +6. Split pure A2UI evaluation or validation code only where boundaries are + natural. + +The first four should be low-risk, behavior-preserving work. Metal, context, and +checkpoint changes require dedicated DS4 parity tests before refactoring.