hugo/DS4Server
1
0
Fork 0
Code Issues 7 Pull Requests Actions 1 Packages Projects Releases Wiki Activity

chore: updated refactor plan

Browse Source
This commit is contained in:
Georg Bauer
2026-07-29 19:14:15 +02:00
parent 6387538da9
commit 95ba6c3c76
1 changed files with 56 additions and 124 deletions
Download Patch File Download Diff File Expand all files Collapse all files

180
REFACTOR.md
View File

@@ -1,159 +1,91 @@
# Refactoring review # Refactoring review
This review is based on a direct reading of the Rust source. It focuses on This is the remaining refactoring backlog after PR #61. Completed work is
idiomatic Rust, duplicate code, and abstractions that do not clearly earn their intentionally omitted. Any implementation must preserve DS4 behavior,
complexity. Any implementation work must preserve DS4 behavior, especially in especially model execution, token processing, context accounting, and KV-cache
model execution, token processing, context accounting, and KV-cache handling. handling.
## Overall assessment ## Current assessment
The project is generally deliberate, concrete Rust. Error propagation is The codebase remains deliberate, concrete Rust. Database message invariants,
usually explicit, ownership is understandable, and the code avoids broad trait runtime submission, checkpoint/source modeling, preference dispatch, and the
or generic frameworks that would obscure DS4 behavior. The main maintainability pure A2UI evaluation and validation boundaries are now concentrated in clear
issue is concentration: several files and state machines are large enough that locations.
duplication and invariants are becoming harder to see.
## 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 Priority: medium
`NewMessage` literals are repeated throughout `src/database.rs`, particularly Preference messages now dispatch through `src/app/preferences.rs`, but
in `start_chat_turn`, `continue_tool_turn`, and `record_compaction`. System, `App::update` still spans roughly 890 lines and handles window lifecycle,
user, tool-result, assistant, and compaction rows repeatedly specify every role projects, generation, A2UI, Git, model management, cache management, and native
flag and optional field. integration.
Add small role-specific constructors such as `system`, `user`, `tool_result`, Move another coherent message family only when its existing `src/app/*` module
and `assistant`. This would centralize role invariants and reduce the chance of can own the complete handling flow. Keep the single public Iced update entry
missing a field when the schema changes. Avoid a generic builder: the semantic point. Avoid controller objects and generic dispatch infrastructure; with the
constructors should make each transaction more explicit, not less. 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 ### 2. Concentrate repeated Metal FFI invariants incrementally
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 Priority: medium, high risk
`src/engine/metal.rs` contains many direct unsafe native calls throughout `src/engine/metal.rs` and `src/engine/metal/glm.rs` still contain many direct
high-level execution logic. Some operations are already wrapped by Rust types, unsafe calls inside high-level execution logic. `src/engine/metal/gpu.rs`
but many call sites still uphold buffer, offset, event, and lifecycle invariants already proves that small wrappers can own repeated tensor, command, and
locally. lifecycle invariants, but coverage is incomplete.
Incrementally add small safe wrappers around proven repeated operations. Do not Add safe wrappers only for repeated, proven operations. Do not introduce a
introduce a generalized GPU framework or reorganize execution merely to reduce general GPU framework or reorganize execution merely to reduce unsafe-block
the number of unsafe blocks. Changes here must be verified against DS4. count. Every change here requires DS4 parity coverage and the relevant real
model/Metal fixtures.
### 7. Review argument-count suppressions selectively ### 3. Replace argument-count suppressions only for existing records
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 Priority: low
`src/a2ui.rs` combines store mutation, streaming parsing, catalog validation, Most remaining suppressions mirror explicit tensor/kernel operands or recursive
function evaluation, formatting, JSON-pointer mutation, and tests. The A2UI view A2UI traversal state and should stay. The clearest remaining candidate is
module is similarly large. 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 Reuse or introduce one concrete statistics record only if it removes that
module boundaries. Avoid arbitrary per-component modules, which would scatter duplication without coupling metrics to the Metal executor. Do not create
the interpreter without reducing conceptual complexity. 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 ### Generated hotlist data
`src/engine/metal/hotlist.rs` is mechanically generated and should not be `src/engine/metal/hotlist.rs` is mechanically generated. Deterministic embedded
treated as hand-written duplication. Keeping deterministic embedded data with data with its import script is preferable to runtime machinery added only to
its import script is preferable to adding runtime machinery merely to reduce reduce source size.
source size.
### Preference-to-engine conversion ### Preference-to-engine conversion
The preference types and effective engine-setting types in `src/settings.rs` The preference types and effective engine-setting types in `src/settings.rs`
serve a useful normalization boundary. Optional user inputs are validated and form a useful normalization boundary: optional user input is validated and
converted into concrete DS4-compatible runtime values. This is useful layering, converted into concrete DS4-compatible runtime values.
not unnecessary abstraction.
### Concrete model descriptions ### Concrete model descriptions
The model `Shape` constants in `src/engine.rs` reuse common values with struct The model `Shape` constants in `src/engine.rs` reuse common values with struct
update syntax while keeping model differences explicit. This is concise and update syntax while keeping model differences explicit.
appropriate for behavior-sensitive model definitions.
## Suggested order ## Suggested order
1. Add semantic `NewMessage` constructors. 1. Extract one more naturally owned `App::update` message family.
2. Consolidate runtime command submission and document metrics policy. 2. Consolidate SSD metrics publication if a single concrete record keeps the
3. Break `App::update` into private domain dispatch methods. boundary simpler.
4. Remove `ResponseKind`. 3. Tighten repeated Metal FFI operations only alongside dedicated DS4 parity
5. Tighten repeated Metal FFI operations incrementally. tests and real fixture runs.
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.