mixeme
4d014e2f45
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 <noreply@anthropic.com>
87 lines
4.1 KiB
Markdown
87 lines
4.1 KiB
Markdown
# 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).
|