diff --git a/README.md b/README.md index 07cdc7f..7403618 100644 --- a/README.md +++ b/README.md @@ -9,6 +9,7 @@ Project notes: - [Changelog](docs/CHANGELOG.md) - [Roadmap](docs/ROADMAP.md) - [Architecture](docs/ARCHITECTURE.md) +- [Refactoring plan](docs/REFACTORING.md) ## Features diff --git a/docs/REFACTORING.md b/docs/REFACTORING.md new file mode 100644 index 0000000..a5fc2a2 --- /dev/null +++ b/docs/REFACTORING.md @@ -0,0 +1,254 @@ +# GoSentry Refactoring Plan + +Status: proposed — not yet started. +Goal: make the codebase **solid**, **comprehensive**, and **human-readable / maintainable** +without changing observable behavior. + +This document is the single source of truth for the refactor. It records the +target architecture, the rationale, and a sequence of small, independently +reviewable tasks. Each task lists the recommended agent model and effort level. + +--- + +## 1. Why refactor + +The application works and is well-commented, but its structure does not scale: + +| # | Problem | Impact | +|---|---------|--------| +| 1 | `src/gui/app.go` is a 1,057-line monolith | Nothing can be found, reused, or tested in isolation | +| 2 | `src/core` is one flat package mixing 7 concerns | No boundaries; everything can call everything | +| 3 | **Shared mutable `*[]Job`** between GUI and `Scheduler` | GUI mutates the slice with no lock; scheduler locks the same slice → data race | +| 4 | `onChange` mutates Fyne widgets **from the scheduler goroutine** | Latent crash/corruption — Fyne requires UI updates on the main thread | +| 5 | `Job` mixes durable config and runtime state (`yaml:"-"` fields) | The "noise" the model fights to exclude lives in the same struct | +| 6 | Errors swallowed everywhere (`_ = store.SaveJobs(...)`) | Save failures are invisible to the user | +| 7 | No service/controller layer; GUI reaches into `store.Paths`, drives scheduler directly | Business logic is tangled into widget callbacks | +| 8 | Schedule strings re-parsed every tick; no `Schedule` value type | Validation scattered; no single source of truth | +| 9 | Tests only cover `core`; GUI and orchestration untestable | Documented gap in `docs/TESTS.md` | + +> Note on layout: the project intentionally **keeps the `src/` directory**. The +> `src/` → `internal/` move was considered and rejected — it is cosmetic for a +> non-imported desktop app and not worth the import-path churn. All packages +> below live under `src/`. + +--- + +## 2. Target architecture + +The central change is to **insert an application-service layer** that owns all +state and exposes intent-based methods. This turns the UI into a thin view and +the core packages into stateless engines, dissolving problems 3, 4, 6, and 7. + +``` +┌──────────────┐ intents ┌─────────────────┐ calls ┌──────────────┐ +│ ui (Fyne) │ ───────────▶ │ app.Service │ ─────────▶ │ core engines │ +│ thin views │ ◀─────────── │ (sole owner of │ │ scheduler / │ +│ fyne.Do only │ events │ state + mutex) │ ◀───────── │ runner / │ +└──────────────┘ └─────────────────┘ records │ storage │ + └──────────────┘ +``` + +- **One writer.** `app.Service` holds the job list + runtime state behind a + mutex. The UI never mutates state directly — it calls `CreateJob`, `RunNow`, + `SetGlobalPause`, etc. +- **Events flow back** through an observer interface. The UI's listener is the + *only* place that touches widgets, and it marshals onto the main thread with + `fyne.Do`. +- **Core engines are stateless / injected** — scheduler and runner operate on + data passed in, not a shared slice. + +### 2.1 Package layout (all under `src/`) + +``` +cmd/gosentry/ + main.go # flag parse → ui.Run + +src/ + domain/ # pure types, zero external deps + job.go # Job (durable config only — no yaml:"-") + runtime.go # JobRuntime (LastRun/NextRun/State/Output/Logs) + record.go # RunRecord + config.go # Config + StartInTrayArgument + schedule.go # Schedule value object: Parse / Validate / Next() + + storage/ # persistence + path resolution + migration + store.go # Load/SaveConfig, Load/SaveJobs + paths.go # ResolvePaths + yaml.go # writeYAML helper + migration.go # pysentry → gosentry legacy handling + + scheduler/ + scheduler.go # timing loop; drives Service via callbacks + clock.go # Clock interface (real + fake for tests) + + runner/ + runner.go # RunJob orchestration + invocation.go # build exec.Cmd (shared) + invocation_windows.go # cmd.exe quoting + invocation_other.go # sh -c + exitcodes.go # parse / accept success codes + logfile.go # writeRunLog + sanitizeFileName + cleanup.go # CleanupLogs + + platform/ + winproc/ # hidden-window helper shared by runner + autostart + winproc_windows.go # CREATE_NO_WINDOW / HideWindow + winproc_other.go # no-op + autostart/ + autostart.go # Manager interface + Status type + windows.go linux.go other.go + desktop/ + desktop_linux.go other.go + + app/ + service.go # owns state; CreateJob/UpdateJob/Delete/RunNow/... + events.go # Event types + Observer registration + format.go # display strings (moved out of GUI) + + ui/ # renamed from src/gui; thin Fyne views + run.go # Run(): lifecycle, window, tray wiring + mainwindow.go # tab assembly + event listener (fyne.Do) + jobs_view.go # list + details panel + toolbar + job_dialog.go # new/edit form + history_view.go # history table + settings_view.go # settings form + tray.go # system tray + singleinstance.go # localhost IPC + layout.go # minWidthLayout +``` + +Import paths follow the existing convention, e.g. +`gitea.mixdep.ru/mix/gosentry/src/domain`, +`gitea.mixdep.ru/mix/gosentry/src/app`. + +### 2.2 Dependency direction (must stay acyclic) + +``` +domain ← (no deps) +storage ← domain +runner ← domain, platform/winproc +scheduler← domain +app ← domain, storage, scheduler, runner +ui ← app, domain (Fyne) +platform/autostart, platform/desktop ← (own deps; winproc for windows) +cmd ← ui +``` + +### 2.3 Key design decisions + +1. **Split durable vs. runtime in the domain.** `domain.Job` becomes pure YAML + config (no `yaml:"-"`). Runtime state moves to `domain.JobRuntime`, held by + the service keyed by job ID. (Resolves #5.) +2. **`Schedule` value object.** `schedule.Parse(string) (Schedule, error)` + validates once and exposes `Next(time.Time)`. (Resolves #8.) +3. **Autostart behind a `Manager` interface**, selected per platform — mockable, + no package-level functions. +4. **Injectable `Clock`** in the scheduler → deterministic tests. +5. **Errors surface to the UI.** Service methods return errors; status bar shows + them. No more `_ =` on saves. (Resolves #6.) +6. **Thread-safety contract:** core engines never import Fyne; the UI listener is + the sole widget mutator and always wraps updates in `fyne.Do`. (Resolves #4.) + +--- + +## 3. Task sequence + +Tasks are ordered so the tree **compiles and all tests pass after every task**. +Each task is a small, reviewable unit. + +**Model guidance** +- `haiku` — mechanical moves, renames, no judgment required. +- `sonnet` — localized logic changes with clear scope. +- `opus` — architecture-shaping work (new layers, concurrency, public APIs). + +**Effort guidance** — reasoning depth, not size: `low` / `medium` / `high`. + +### Phase 0 — Safety net + +| Task | Description | Model | Effort | +|------|-------------|-------|--------| +| T0.1 | Add `scripts/test.sh` + `.bat` running `go vet ./...` and `go test -race ./...`. Document in `docs/TESTS.md`. | haiku | low | +| T0.2 | Add characterization tests that pin current behavior at seams to be moved: store load→save round-trip, scheduler `nextRunTime`, end-to-end `RunJob` log output. (Some exist; fill gaps.) | sonnet | medium | + +### Phase 1 — Split the flat `core` package (no logic change) + +Mechanical moves + import fixes only. Behavior identical. + +| Task | Description | Model | Effort | +|------|-------------|-------|--------| +| T1.1 | Create `src/domain`; move `Job`, `RunRecord`, `Config`, `JobsFile`, `StartInTrayArgument` from `model.go`. Keep `yaml:"-"` fields for now (split happens in Phase 2). Update all references. | sonnet | medium | +| T1.2 | Create `src/platform/winproc`; move `configureHiddenWindow` + hidden-window flags out of `runner_windows.go` / `runner_other.go`. This breaks the future autostart→runner coupling early. | sonnet | medium | +| T1.3 | Create `src/runner`; move `runner.go`, `runner_windows.go`, `runner_other.go`, `runner_test.go`. Point at `winproc`. Split helpers into `invocation*.go`, `exitcodes.go`, `logfile.go`, `cleanup.go` as the file moves. | sonnet | medium | +| T1.4 | Create `src/scheduler`; move `scheduler.go`, `scheduler_test.go`. Still takes `*[]domain.Job` for now. | sonnet | medium | +| T1.5 | Create `src/storage`; move `store.go`, `paths.go`, `store_test.go`. | sonnet | medium | +| T1.6 | Create `src/platform/autostart`; move `autostart_*.go` + tests. Point at `winproc`. | sonnet | medium | +| T1.7 | Create `src/platform/desktop`; move `desktop_linux.go`, `desktop_other.go`. | haiku | low | +| T1.8 | Delete the now-empty `src/core`; run full build + tests on both platforms (or with build tags) to confirm parity. | haiku | low | + +### Phase 2 — Domain cleanup + +| Task | Description | Model | Effort | +|------|-------------|-------|--------| +| T2.1 | Add `src/domain/schedule.go`: `Schedule` value object with `Parse`, `Validate`, `Next(time.Time)`. Unit-test it. Keep `nextRunTime` as a thin wrapper initially. | opus | high | +| T2.2 | Migrate `scheduler` to use `Schedule` (parse on load/edit, not per tick). Remove duplicated parsing. | sonnet | medium | +| T2.3 | Split `domain.Job` (durable) from `domain.JobRuntime` (transient). Remove all `yaml:"-"` fields and `nextDue` from `Job`. Add `runtime.go`. | opus | high | +| T2.4 | Update `storage`: load/save only `Job`; move runtime initialization out of `normalizeJobs` into a `domain.NewRuntime(job)` constructor. Update round-trip tests. | sonnet | medium | + +> After Phase 2 the scheduler and GUI still share state; the `Job`/`JobRuntime` +> split is wired through temporary glue. Phase 3 removes the sharing. + +### Phase 3 — Application service layer + +| Task | Description | Model | Effort | +|------|-------------|-------|--------| +| T3.1 | Create `src/app/service.go`: `Service` owning `[]domain.Job` + `map[int]*domain.JobRuntime` behind a `sync.Mutex`. Constructor wires `storage`. | opus | high | +| T3.2 | Add `src/app/events.go`: `Event` types (job changed, run recorded, scheduler state) + `Observer` registration. Single-threaded dispatch contract documented. | opus | high | +| T3.3 | Move state-mutating operations into the service: `CreateJob`, `UpdateJob`, `DeleteJob`, `SetEnabled`, `RunNow`, `SetGlobalPause`, `UpdateSettings`. Each returns `error`. | opus | high | +| T3.4 | Convert `scheduler` to operate through the service (no `*[]Job`). Scheduler asks the service for due jobs and reports records back; service is the sole writer. Inject `Clock`. | opus | high | +| T3.5 | Move display/format helpers (`displayFolder`, `displayArguments`, `displayRunMode`, `statusText`, …) from GUI into `src/app/format.go`. | haiku | low | +| T3.6 | Add `src/app` unit tests (no Fyne): create/edit/delete, enable/pause, global pause, run-now path with a fake runner + fake clock. Big coverage win. | opus | high | + +### Phase 4 — Carve up the GUI + +Rename `src/gui` → `src/ui` and break `app.go` into focused files. The UI now +talks only to `app.Service` and reacts to events via `fyne.Do`. + +| Task | Description | Model | Effort | +|------|-------------|-------|--------| +| T4.1 | Rename package `gui` → `ui`; split lifecycle into `run.go` + `mainwindow.go`. Wire the event listener and route every widget update through `fyne.Do`. (Resolves #4.) | opus | high | +| T4.2 | Extract `jobs_view.go` (list + details + toolbar), driven by service calls + events. | sonnet | medium | +| T4.3 | Extract `job_dialog.go`; validate schedule via `domain.Schedule.Validate`. | sonnet | medium | +| T4.4 | Extract `history_view.go`. | sonnet | medium | +| T4.5 | Extract `settings_view.go`; surface save/autostart/cleanup errors to the status label. (Resolves #6 in UI.) | sonnet | medium | +| T4.6 | Extract `tray.go`, `singleinstance.go`, `layout.go`. | haiku | low | +| T4.7 | Confirm `app.go` is gone and `ui` imports only `app` + `domain` + Fyne. Manual smoke test on each platform. | sonnet | medium | + +### Phase 5 — Hardening & docs + +| Task | Description | Model | Effort | +|------|-------------|-------|--------| +| T5.1 | Replace remaining `_ = ...Save...` with propagated/surfaced errors across service + storage. | sonnet | medium | +| T5.2 | Introduce `autostart.Manager` interface + per-platform impls; inject into the service instead of calling package funcs. | sonnet | medium | +| T5.3 | Fill documented test gaps: folder filtering, log cleanup (count + age), settings persistence/migration, concurrent run prevention. | sonnet | high | +| T5.4 | Run `go test -race ./...` clean. Confirm no data race remains. | haiku | low | +| T5.5 | Update `docs/ARCHITECTURE.md`, `docs/TESTS.md`, and the README "Project Layout" section to the new structure. | sonnet | medium | + +--- + +## 4. Definition of done + +- `go vet ./...` clean; `go test -race ./...` green on Windows and Linux. +- No package outside `ui` imports Fyne; no engine mutates UI state. +- `domain.Job` has no `yaml:"-"` fields. +- `app.Service` is the only writer of job/runtime state. +- `src/ui` contains no file over ~250 lines; no single file over ~400. +- `docs/ARCHITECTURE.md` matches the shipped structure. + +## 5. Risks & mitigations + +| Risk | Mitigation | +|------|-----------| +| Cross-platform code moves break the non-host OS build | Build with both `GOOS=windows` and `GOOS=linux` after each platform-touching task (T1.2, T1.3, T1.6, T1.7). | +| Concurrency change (Phase 3/4) introduces subtle deadlocks | Keep the service mutex non-reentrant; never call back into the UI while holding it; cover with `-race` tests in T3.6. | +| Behavior drift during moves | Characterization tests (T0.2) pin behavior before structural change. | +| Large diff hard to review | Each task is a separate commit/PR; phases land independently. |