160 lines
6.4 KiB
Markdown
160 lines
6.4 KiB
Markdown
# 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.
|