Document refactoring opportunities

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.