From 95ba6c3c76e08a4d473dd4016531193f26c4ff33 Mon Sep 17 00:00:00 2001 From: Georg Bauer Date: Wed, 29 Jul 2026 19:14:15 +0200 Subject: [PATCH] chore: updated refactor plan --- REFACTOR.md | 180 ++++++++++++++++------------------------------------ 1 file changed, 56 insertions(+), 124 deletions(-) diff --git a/REFACTOR.md b/REFACTOR.md index 576f927..5524708 100644 --- a/REFACTOR.md +++ b/REFACTOR.md @@ -1,159 +1,91 @@ # 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. +This is the remaining refactoring backlog after PR #61. Completed work is +intentionally omitted. Any implementation must preserve DS4 behavior, +especially model execution, token processing, context accounting, and KV-cache +handling. -## Overall assessment +## Current 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. +The codebase remains deliberate, concrete Rust. Database message invariants, +runtime submission, checkpoint/source modeling, preference dispatch, and the +pure A2UI evaluation and validation boundaries are now concentrated in clear +locations. -## Recommended work +The remaining concentration is mostly in `App::update` and the Metal execution +path. Neither justifies a broad framework: keep extracting only boundaries that +already exist in the domain, and treat Metal changes as high-risk DS4 parity +work. -### 1. Add semantic constructors for database messages +## Remaining work + +### 1. Continue reducing `App::update` at owned domain boundaries 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. +Preference messages now dispatch through `src/app/preferences.rs`, but +`App::update` still spans roughly 890 lines and handles window lifecycle, +projects, generation, A2UI, Git, model management, cache management, and native +integration. -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. +Move another coherent message family only when its existing `src/app/*` module +can own the complete handling flow. Keep the single public Iced update entry +point. Avoid controller objects and generic dispatch infrastructure; with the +current flat `Message` enum, extra pass-through matchers also weaken exhaustive +matching and should earn that cost. -### 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 +### 2. Concentrate repeated Metal FFI invariants incrementally 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. +`src/engine/metal.rs` and `src/engine/metal/glm.rs` still contain many direct +unsafe calls inside high-level execution logic. `src/engine/metal/gpu.rs` +already proves that small wrappers can own repeated tensor, command, and +lifecycle invariants, but coverage is incomplete. -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. +Add safe wrappers only for repeated, proven operations. Do not introduce a +general GPU framework or reorganize execution merely to reduce unsafe-block +count. Every change here requires DS4 parity coverage and the relevant real +model/Metal fixtures. -### 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 +### 3. Replace argument-count suppressions only for existing records 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. +Most remaining suppressions mirror explicit tensor/kernel operands or recursive +A2UI traversal state and should stay. The clearest remaining candidate is +metrics publication: `Metrics::ssd_stats` accepts nineteen fields that already +travel together as execution statistics, and its call is duplicated in +`src/engine.rs`. -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. +Reuse or introduce one concrete statistics record only if it removes that +duplication without coupling metrics to the Metal executor. Do not create +one-use parameter objects for `Generator::generate`, `Generator::compact`, +Metal kernels, or A2UI rendering solely to satisfy Clippy. -## No refactoring recommended +## Leave alone ### 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. +`src/engine/metal/hotlist.rs` is mechanically generated. Deterministic embedded +data with its import script is preferable to runtime machinery added only 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. +form a useful normalization boundary: optional user input is validated and +converted into concrete DS4-compatible runtime values. ### 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. +update syntax while keeping model differences explicit. ## 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. +1. Extract one more naturally owned `App::update` message family. +2. Consolidate SSD metrics publication if a single concrete record keeps the + boundary simpler. +3. Tighten repeated Metal FFI operations only alongside dedicated DS4 parity + tests and real fixture runs.