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>
4.1 KiB
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 and 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? 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 — 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, or — when the work is deferred rather than decided — a note in 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 so it is not re-reported.
- Work larger than a single fix → ROADMAP.md, with the reasoning.
- A new rule the review establishes → STANDARDS.md.