ddidderr
41e9a0efc1
Replace the `a9f9845` local-update dedup cache with explicit peer event semantics. Local scans now emit `LocalLibraryChanged` when the library changes, while operation mutations emit `ActiveOperationsChanged` from the mutation path. Tauri keeps joining those facts into the existing `games-list-updated` payload, so the frontend contract stays stable. This removes the cache/invalidation coupling between scan emission and operation state. The remaining forced local snapshot is explicit: accepted game directory changes can refresh the UI for an equivalent new path without sending a peer library delta. Operation guard cleanup and liveness cancellation now publish the same active operation snapshot as normal command-handler transitions. The peer CLI JSONL events follow the same split with `local-library-changed` and `active-operations-changed`. Test Plan: - `just fmt` - `CARGO_BUILD_RUSTC_WRAPPER= just test` - `CARGO_BUILD_RUSTC_WRAPPER= just clippy` - `git diff --check` Refs: CLEAN_CODE_PLAN_1.md
77 lines
3.3 KiB
Markdown
77 lines
3.3 KiB
Markdown
# Clean code notes
|
|
|
|
Running notes on architectural smells in the codebase: things that work today
|
|
but are shaped wrong, and what a cleaner design would look like. Each entry
|
|
should explain the smell, why the current shape exists, and the warning sign
|
|
that says "now is the time to refactor."
|
|
|
|
---
|
|
|
|
## Resolved: local library and operation UI signals are split
|
|
|
|
**Context.** Commit `a9f9845` ("fix(peer): suppress duplicate local game
|
|
updates") added a `last_local_update_key` cache in `Ctx`, keyed on
|
|
`(revision, digest, active_operations)`. That worked, but it made scan emission
|
|
responsible for deduplicating operation-state changes that were actually owned
|
|
by command handlers.
|
|
|
|
### The root issue
|
|
|
|
`update_and_announce_games` already knows whether the library actually changed:
|
|
`LocalLibraryState::update_from_scan` returns `Option<LibraryDelta>`, where
|
|
`None` means "nothing changed." That fact gates peer `LibraryDelta`
|
|
announcements. Active operation status has a different source of truth:
|
|
`Ctx::active_operations`, mutated by operation start, handoff, end, liveness
|
|
cancellation, and guard cleanup.
|
|
|
|
The old `LocalGamesUpdated { games, active_operations }` event mixed those two
|
|
sources of truth. The dedup key was a symptom of that overload.
|
|
|
|
### Current shape
|
|
|
|
The peer runtime now emits two separate facts:
|
|
|
|
- `LocalLibraryChanged { games }` is emitted from scans when the local library
|
|
state changes. A real `SetGameDir` path change may force one local snapshot
|
|
for the UI even when the library digest matches the previous path, because
|
|
Tauri has already cleared local flags for the old path.
|
|
- `ActiveOperationsChanged { active_operations }` is emitted when the operation
|
|
table changes. Normal mutations go through `begin_operation`,
|
|
`transition_download_to_install`, and `end_operation`; liveness cancellation
|
|
and `OperationGuard` cleanup publish the same snapshot when they clear state.
|
|
|
|
Tauri is the join boundary. It stores the latest game DB and latest active
|
|
operation snapshot, then keeps emitting the existing frontend
|
|
`games-list-updated` payload. The frontend did not need to learn the peer
|
|
runtime's internal event split.
|
|
|
|
### Invariants to protect
|
|
|
|
Do not reintroduce a scan-level dedup cache for operation state. If a new path
|
|
mutates `Ctx::active_operations`, route it through the operation publisher or
|
|
explicitly document why it is not UI-visible. If a new local scan reason needs a
|
|
UI snapshot without a peer delta, model that as an explicit scan policy like the
|
|
path-change forced snapshot, not as cache invalidation.
|
|
|
|
---
|
|
|
|
## How to add to this file
|
|
|
|
When a code review uncovers a "this works but it's bolted on" pattern, write it
|
|
up here. Structure:
|
|
|
|
1. **Context** — what commit/PR introduced the pattern, one-paragraph summary.
|
|
2. **Root issue** — the underlying invariant the code is working around.
|
|
3. **Why the obvious fix doesn't work** — what constraints forced the current
|
|
shape.
|
|
4. **The tell** — concrete code shapes (scattered invalidations, repeated
|
|
special cases, dedup keys that re-derive existing facts) that signal the
|
|
smell.
|
|
5. **Clean shape** — what the code would look like without the constraint.
|
|
6. **Warning signs** — what observations in future work mean "do the
|
|
refactor now."
|
|
|
|
Keep entries narrative, not bulleted to death. The point is to preserve the
|
|
_reasoning_ so future contributors can decide whether the trade-off still
|
|
holds.
|