From 4d014e2f45002fe0116b1664190f69ebcabb3c36 Mon Sep 17 00:00:00 2001 From: mixeme Date: Sun, 26 Jul 2026 22:37:45 +0300 Subject: [PATCH] docs: add a review agenda and config compatibility rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewing the project used to mean re-stating what to look at every time. docs/REVIEW.md now holds that agenda once — nine areas, each anchored to this codebase — and both entry points point at it rather than copying it: the /review-project command in .claude/commands, and a section in CLAUDE.md so a plain-language review request lands in the same place. STANDARDS.md gains a "Config file compatibility" section. The project has applied the same rule three times (Theme, JobListView, TimeoutSeconds) without ever writing it down: a new Config field is omitempty and its zero value means the previous behavior, a meaningful zero is never backfilled on load, and an unrecognised enum value reads as the default through one shared helper. With no migration step and hand-editable files, that is what keeps older configs working. Also removes docs/PLAN-compact-job-list.md, implemented in edabc57 — everything but the version bump, which now waits for the release along with the rest of the Unreleased section. .claude/settings.local.json is ignored so the shared command can be tracked without per-developer permissions. Co-Authored-By: Claude Opus 5 --- .claude/commands/review-project.md | 26 +++++ .gitignore | 4 + CLAUDE.md | 8 +- README.md | 1 + docs/PLAN-compact-job-list.md | 151 ----------------------------- docs/REVIEW.md | 86 ++++++++++++++++ docs/STANDARDS.md | 22 ++++- 7 files changed, 145 insertions(+), 153 deletions(-) create mode 100644 .claude/commands/review-project.md delete mode 100644 docs/PLAN-compact-job-list.md create mode 100644 docs/REVIEW.md diff --git a/.claude/commands/review-project.md b/.claude/commands/review-project.md new file mode 100644 index 0000000..e96a748 --- /dev/null +++ b/.claude/commands/review-project.md @@ -0,0 +1,26 @@ +--- +description: Review the project as a whole against the agenda in docs/REVIEW.md +--- + +Perform a whole-project review of GoSentry. + +Read [docs/REVIEW.md](../../docs/REVIEW.md) first — it is the agenda, and its +nine sections are the areas to cover. Read [docs/STANDARDS.md](../../docs/STANDARDS.md) +and [docs/ARCHITECTURE.md](../../docs/ARCHITECTURE.md) for the rules and +contracts the code is checked against. + +$ARGUMENTS narrows the review when given — a package path, a file, or the name +of an agenda section. With no arguments, sweep the whole `src/` tree. + +Rules for the report: + +- Anything listed under "Intentional behavior" in STANDARDS.md is not a finding. + If you believe such an entry is now wrong, say so explicitly as a challenge to + the decision rather than reporting it as a bug. +- Verify before reporting. Read the surrounding code and, where cheap, confirm + the behavior with a test rather than reasoning about it alone. +- Group findings by agenda section, most severe first, each with the file and + line and what would actually go wrong. +- Report honestly that a section is clean rather than inventing something for it. +- Do not fix anything during the review. Report first; apply fixes only when + asked, following "What happens to the findings" in REVIEW.md. diff --git a/.gitignore b/.gitignore index 3ab05ef..39d1e1e 100644 --- a/.gitignore +++ b/.gitignore @@ -20,6 +20,10 @@ logs/ go.work go.work.sum +# Claude Code per-developer settings. The shared command in .claude/commands is +# tracked; locally granted tool permissions are not. +.claude/settings.local.json + # GoodSync metadata. This is intentionally kept because the directory is local # to the user's file synchronization setup. _gsdata_/ diff --git a/CLAUDE.md b/CLAUDE.md index c2f6f3c..8157e46 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -12,6 +12,12 @@ application service, scheduler, storage, and command runner in one binary. - [docs/TESTS.md](docs/TESTS.md) — test layout and conventions. - [docs/ROADMAP.md](docs/ROADMAP.md) — deliberately out of scope. +## Reviewing the project + +When the user asks for a review of the project (rather than of a specific +diff), follow [docs/REVIEW.md](docs/REVIEW.md) — it is the agenda, and the +`/review-project` command runs the same thing. Do not improvise a checklist. + ## Key rules (full list in STANDARDS.md) - `src/app.Service` is the sole owner of job and runtime state; the UI reads it @@ -22,7 +28,7 @@ application service, scheduler, storage, and command runner in one binary. a regression test. - UI view constructors accept an injected `*app.Service`; `app.Open()` is called only from `run.go`. -- Off-main-thread widget updates must go through `fyne.Do` (Fyne v2.6.x). +- Off-main-thread widget updates must go through `fyne.Do` (Fyne v2.7.4). ## Build and test diff --git a/README.md b/README.md index 1ac6173..728651b 100644 --- a/README.md +++ b/README.md @@ -49,6 +49,7 @@ GoSentry is built and tested on **Windows** and **Linux**: - [Roadmap](docs/ROADMAP.md) — planned work larger than a single bug fix - [Architecture](docs/ARCHITECTURE.md) — component interaction model - [Standards](docs/STANDARDS.md) — quality rules and intentional behavior +- [Review](docs/REVIEW.md) — what a whole-project review looks at - [Development](docs/DEVELOPMENT.md) — build instructions, project layout, dependencies - [Tests](docs/TESTS.md) — test suite layout and how to run it - [Performance](docs/PERFORMANCE.md) — measured performance findings diff --git a/docs/PLAN-compact-job-list.md b/docs/PLAN-compact-job-list.md deleted file mode 100644 index 90afe94..0000000 --- a/docs/PLAN-compact-job-list.md +++ /dev/null @@ -1,151 +0,0 @@ -# Implementation plan — compact / detailed job list view - -## Context - -The Jobs tab sidebar renders every job as a three-line block (bold name, a -metadata line with folder + schedule + command, and a status line) — see the -`widget.NewList` call in [jobs_view.go:108](../src/ui/jobs_view.go). That is -informative but tall: with many jobs the sidebar needs constant scrolling. - -Add a second, opt-in rendering of the same list: **Compact** shows one line per -job (name left, status right); **Detailed** is the current three-line block and -stays the default. The choice is switchable from the Jobs tab itself and is -persisted in `gosentry.json`, so it survives a restart like `Config.Theme` does. - -## Design decisions - -- **Control lives in the Jobs sidebar header** — a toggle button on the same row - as the existing "Folder" filter, to its right. Switching is one click, next to - what it affects. Fyne 2.7.4 has no dedicated toggle widget, so the button - flips its own text and icon on tap, exactly like `stopAllButton` - ("Disable auto" / "Enable auto") already does in `src/ui/jobs_view.go`. Like - that button it is labelled with the *action*, not the current state: - `Compact` + `theme.ListIcon()` while detailed, `Detailed` + - `theme.ViewFullScreenIcon()` while compact. The toolbar row (New job / Edit / - Run now / Pause / Delete) is left untouched. -- **Compact row = name + status** on a single line. Same height as one label, - so per-job health stays visible at a glance. -- **One list, one row template.** `widget.List` caches the row template's - `MinSize` as `itemMin`; `list.Refresh()` re-creates the template and - recomputes it (verified in Fyne 2.7.4 `widget/list.go`, - `listRenderer.Refresh`), so the rows genuinely shrink/grow on a mode switch - without rebuilding the widget. The template therefore always holds the same - four objects, and the mode is expressed by `Show()`/`Hide()`. Both - `compactVBoxLayout` (`src/ui/layout.go`) and Fyne's border layout skip hidden - children in `MinSize`, so a hidden line contributes no height. - -## Changes - -### 1. `src/domain/config.go` — persisted preference - -- New string type `JobListView` with `JobListViewDetailed = "detailed"` and - `JobListViewCompact = "compact"`, documented like `Theme` as a UI-only choice. -- New field `JobListView JobListView \`json:"job_list_view,omitempty"\`` on - `Config`; comment that empty means detailed so pre-existing configs keep the - current look. -- `DefaultConfig()` returns `JobListViewDetailed`. -- Method `func (v JobListView) IsCompact() bool` — only the exact `"compact"` - value is compact — so every consumer normalizes unknown/legacy values the - same way. - -### 2. `src/app/operations.go` — save the choice - -New `func (s *Service) SetJobListView(view domain.JobListView) error`, modelled -on `SetGlobalPause` (`src/app/operations.go:168`) but deliberately lighter: -normalize a non-`compact` value to `JobListViewDetailed`, take `mu`, return -early if unchanged, set `s.store.Config.JobListView`, call -`s.store.SaveConfig()`, unlock. No `SaveJobs` (no job changed), no event emitted -(this is presentational only — the Jobs view refreshes its own list; an event -would trigger a pointless whole-window refresh). A comment records that -reasoning. - -### 3. `src/ui/jobs_view.go` — rendering and the toggle - -- Read the initial mode into a `compactList` local: - `svc.Store().Config.JobListView.IsCompact()`. -- **CreateItem**: build `name` (bold, `Wrapping = fyne.TextTruncate` so a long - name cannot push the status off the row), `inlineStatus` (compact-only, - right-hand side), `meta`, and `status`. Put the first two on one line via - `container.NewBorder(nil, nil, nil, inlineStatus, name)` and keep the outer - `compactVBoxLayout{spacing: jobRowSpacing}` container with - `[nameLine, meta, status]`. Apply the mode's visibility here too — this is - what makes `itemMin` correct for the mode. -- **UpdateItem**: reach `nameLine` through `row.Objects[0].(*fyne.Container)` - (`NewBorder` appends the border slots after the center object, so - `Objects[0]` is `name` and `Objects[1]` is `inlineStatus` — worth a comment, - matching the existing index-based access in this callback). Set all four texts - from the existing helpers (`app.DisplayFolder`, `app.DisplayInvocation`, - `app.StatusText`) and **re-apply visibility on every update**: a full - `Refresh` reuses the already-visible row objects, which were created under the - old mode, so visibility cannot be left to `CreateItem` alone. -- **View toggle button**: `viewButton := widget.NewButtonWithIcon(…)` built with - the text/icon for the current mode. Its `OnTapped` flips the mode - (`nextJobListView`), persists via `svc.SetJobListView` — on error - `dialog.ShowError` and roll the local mode back without touching the button, - per the error rule in `docs/STANDARDS.md` — then applies the new - `SetText`/`SetIcon` and calls `list.Refresh()`. This mirrors `stopAllButton`'s - flip-and-revert handler in the same file. The list selection and the details - panel are untouched by the switch. -- **Header layout**: keep the `"Folder"` caption and `folderSelect` exactly as - they are, and put the button beside the select on the same row — - `container.NewBorder(nil, nil, nil, viewButton, folderSelect)`. The border - layout gives the button its `MinSize` on the right and lets the select fill - the rest, so the header gains no height and the toolbar row is not touched. - -### 4. `src/ui/jobs_view_helpers.go` — button state helpers - -Two pure helpers so the on-disk strings never reach the user and the button's -wording is testable without a running GUI: - -- `nextJobListView(current domain.JobListView) domain.JobListView` — flips the - mode, treating anything that is not `compact` as detailed (via `IsCompact`). -- `viewToggleText(current domain.JobListView) string` — the action label: - `"Compact"` while detailed, `"Detailed"` while compact. - -The icon choice stays inline at the button, next to `SetText`/`SetIcon`, so the -helpers file keeps its "no widget imports" character. - -### 5. Tests - -- `src/domain/config_test.go` (new): `JobListView.IsCompact` for `"compact"`, - `"detailed"`, `""`, and a junk value. -- `src/ui/jobs_view_test.go`: `nextJobListView` flips both ways and maps an - empty/unknown value to compact (since such a value reads as detailed), and - `viewToggleText` returns the action label for each mode. -- `src/app/operations_test.go`: `TestSetJobListViewPersistsToConfigFile` - modelled on `TestSetGlobalPausePersistsToConfigFile` - (`src/app/operations_test.go:553`) — switch to compact, unmarshal - `svc.store.Paths.ConfigPath`, assert the field, switch back; plus a case - asserting an unknown value is stored as `detailed`. -- `src/storage/store_test.go`: one added assertion in the defaults test beside - the existing `Theme` check (~line 174) that a fresh config is `detailed`. - -### 6. Docs and version - -- `docs/CHANGELOG.md`: new `## 0.14.0 - 2026-07-26` section (a feature → minor - bump) covering the compact/detailed toggle and the new `job_list_view` field, - noting legacy configs stay detailed. -- `src/app/version.go`: `0.13.0` → `0.14.0`. -- `docs/ARCHITECTURE.md`: add the two new helpers to the `jobs_view_helpers.go` - row of the file-split table (~line 195). - -## Verification - -1. Build, vet, and test the whole module through the cgo-enabled shell (the - default Bash env has CGO off, so GUI packages will not compile there): - -```bash -export PATH="/c/msys64/ucrt64/bin:$PATH"; export CGO_ENABLED=1; go build ./... && go vet ./... && go test -race ./... -``` - -2. Run the app (`go run ./cmd/gosentry`) with several jobs configured and check: - - Default launch is Detailed and looks exactly as before; the button reads - "Compact" and sits to the right of the folder filter without making the - header taller. - - Tapping it collapses every row to one line, name left / status right, many - more jobs fit without scrolling, and the button now reads "Detailed". - - Selection and the details panel keep working after switching, in both - directions, including with a folder filter active and with an empty list. - - A running job's status still updates live in compact rows. - - `gosentry.json` gains `"job_list_view": "compact"`; restarting reopens in - compact, and switching back removes/flips the field. diff --git a/docs/REVIEW.md b/docs/REVIEW.md new file mode 100644 index 0000000..d5ad5cb --- /dev/null +++ b/docs/REVIEW.md @@ -0,0 +1,86 @@ +# GoSentry — Review Agenda + +What to look at when reviewing the project as a whole, as opposed to a single +diff. This is the agenda; the rules a review checks against live in +[STANDARDS.md](STANDARDS.md) and [ARCHITECTURE.md](ARCHITECTURE.md). + +Scope note: a normal pull-request review checks the change. This agenda is for +a periodic sweep of the whole codebase, so a pass may legitimately end with +"nothing to report" on most items. + +## 1. Architecture and project structure + +Does the code still match the package map and the event flow in +[ARCHITECTURE.md](ARCHITECTURE.md)? Watch for the boundaries that matter here: +`app.Service` as the sole owner of job and runtime state, the UI reading it +through typed events, `domain` staying free of I/O, and platform-specific code +staying behind the `platform/*` interfaces. + +## 2. Complexity against the size of the project + +GoSentry is a single-process desktop app with two direct dependencies. Flag +abstraction that is not paying for itself: interfaces with one implementation +and no test seam, indirection added for a use case nobody has asked for, a new +dependency where thirty lines of standard library would do. Also check the +opposite direction — files that have grown past the size guideline in +ARCHITECTURE and should be split the way `jobs_view.go` was. + +## 3. Code quality + +The checkable rules are in [STANDARDS.md](STANDARDS.md) — error handling, unit +tests for pure helpers, regression tests for fixes, `fyne.Do` for updates off +the main thread. Beyond them: concurrency around `Service.mu`, goroutines whose +lifetime is not obvious, and error paths that report something less useful than +what they caught. + +## 4. Documentation and comments + +Does every documented behavior still exist, and does every non-obvious behavior +get documented? Check the doc set against the code: README (user-facing +behavior and config keys), ARCHITECTURE (packages and flows), STANDARDS +(rules and intentional behavior), DEVELOPMENT (build), TESTS, PERFORMANCE, +CHANGELOG (an entry per notable change). For comments, the bar is *why*, not +*what* — a comment restating the line below it is noise; an unexplained +workaround is a finding. + +## 5. Readability and maintainability + +Read a package as someone who has not seen it before. Can the next change be +made without reverse-engineering? Naming that matches the domain vocabulary, +functions that do one thing, and control flow that does not need a diagram. + +## 6. Logical errors + +Correctness independent of style: scheduling and timing edge cases (overlap +policy, sequential mode, pause interactions), off-by-one and boundary handling, +zero values that mean something (see the timeout rules in STANDARDS), state +that can be observed mid-update, and error paths that leave state inconsistent. + +## 7. Legacy code and migrations + +The app has no database, so migration means file compatibility: `gosentry.json` +and `jobs.json` written by an older version must keep working. Check that new +`Config` fields are backward compatible, that normalization happens in one +place, and that values which are meaningful zeros are not normalized away. Also +look for code kept alive only for a case that no longer exists. + +## 8. Undocumented or under-documented contentious decisions + +Any decision a future reader would question needs its reasoning recorded where +it lives: a comment at the code, an entry in the "Intentional behavior" section +of [STANDARDS.md](STANDARDS.md), or — when the work is deferred rather than +decided — a note in [ROADMAP.md](ROADMAP.md), which is where the frozen +window-size work keeps its rationale. + +## 9. Other improvement proposals + +Anything that does not fit above: build and release ergonomics, test coverage +gaps, dependency health, UX rough edges. + +## What happens to the findings + +- A defect → fix it, with a regression test when severity is medium or higher. +- Behavior that turns out to be deliberate → record it under "Intentional + behavior" in [STANDARDS.md](STANDARDS.md) so it is not re-reported. +- Work larger than a single fix → [ROADMAP.md](ROADMAP.md), with the reasoning. +- A new rule the review establishes → [STANDARDS.md](STANDARDS.md). diff --git a/docs/STANDARDS.md b/docs/STANDARDS.md index f77f40c..b9d17f3 100644 --- a/docs/STANDARDS.md +++ b/docs/STANDARDS.md @@ -1,7 +1,8 @@ # GoSentry — Standards Quality rules and intentional behavior for contributors. Package contracts live -in [ARCHITECTURE.md](ARCHITECTURE.md); test conventions in [TESTS.md](TESTS.md). +in [ARCHITECTURE.md](ARCHITECTURE.md); test conventions in [TESTS.md](TESTS.md); +what a whole-project review looks at, in [REVIEW.md](REVIEW.md). ## Code quality @@ -12,6 +13,25 @@ in [ARCHITECTURE.md](ARCHITECTURE.md); test conventions in [TESTS.md](TESTS.md). - Documented intentional behavior → section below, not a backlog bug. - UI view constructors accept `*app.Service`; call `app.Open()` only from `run.go`. +## Config file compatibility + +There is no migration step: `gosentry.json` and `jobs.json` are read as-is, are +meant to be hand-editable, and may have been written by an older version. A +change to their shape has to stay compatible on its own. + +- A new `Config` field is tagged `omitempty`, and its zero value must mean the + behavior that existed before the field was added — a file written without it + keeps working unchanged. `DefaultConfig()` still sets the value explicitly. +- A zero that carries meaning is not a missing field and must not be backfilled + on load. See `DefaultTimeoutSeconds` in `storage.loadOrCreateConfig` and + `Job.TimeoutSeconds *int`, where unset and `0` are different answers. +- An unrecognised enum value reads as the default rather than an error, through + one helper that every consumer shares (`JobListView.IsCompact`, `ui.themeFor`), + and is normalized before being written back, so the file never gains a value + no reader understands. +- Each of the three gets a test: the default in `storage`, the normalization in + `domain`, and a round-trip through the real config file in `app`. + ## Intentional behavior (not bugs) - `RunNow` is allowed during global pause and for disabled jobs.