mix/gosentry
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Add refactoring plan document

Browse Source 
Document a phased plan to restructure GoSentry into focused packages
under src/ (domain, storage, runner, scheduler, platform, app, ui) with
an application-service layer that owns state, and link it from the README.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
mixeme
2026-06-18 08:16:03 +03:00
parent d24211cab2
commit 4c49104cce
2 changed files with 255 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
README.md
+1
View File
@@ -9,6 +9,7 @@ Project notes:
- [Changelog](docs/CHANGELOG.md)
- [Roadmap](docs/ROADMAP.md)
- [Architecture](docs/ARCHITECTURE.md)
- [Refactoring plan](docs/REFACTORING.md)
## Features
docs/REFACTORING.md
+254
View File
@@ -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. |