docs: fix three drifted claims in ARCHITECTURE.md - #141
Merged
Merged
Conversation
- The dashboard never polls /api/v1/alerts (grep of app.js confirms it); the alert engine has no dashboard surface yet, tracked separately as issue #11. Clarify that the card coloring is threshold-based, not alert-state-based, so the two aren't conflated. - The HTTP layer section omitted withGzip entirely, even though it sits on every /api/v1 request path (added in #95). Show the global vs. per-route (apiRoute) middleware chains explicitly, and add a dedicated withGzip bullet. - "exactly four collaborators" was followed by five numbered items; drop the count rather than keep it in sync with the list. Docs-only change; no source touched, no new tests needed per docs/TESTS.md.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
📖 Description
docs/ARCHITECTURE.mdhad drifted from the code in three places, all corrected in this PR:/api/v1/alerts. Verified viagrep -n "api/v1" internal/web/assets/app.json the currentmain— no such reference exists. The alert engine (GET /api/v1/alerts) is fully implemented server-side but has no dashboard surface at all yet; that gap is tracked separately by C3: Surface alert status in the dashboard UI #11. Rewrote the "Web dashboard" section to state this explicitly and to stop conflating alert-state with the (real,>=-based) threshold coloring of metric cards.withGzipmissing from the "HTTP layer" section entirely, even though it sits on every/api/v1/...request path (added in Compress /api/v1 responses with gzip #95, documented indocs/API.md#compression). The section previously showed only the globalwithLogging(withSecurityHeaders(mux))chain. I re-verified the current code (internal/httpapi/server.go,apiRoute) and found the per-route chain has grown since the issue was filed — it's nowwithNoStore(withMaxInFlight(withGzip(withAPIKey(handler)))), not justwithGzip(withAPIKey(handler)). Updated the doc to show both the global and per-route chains explicitly and added a dedicatedwithGzipbullet (gzip negotiation,sync.Poolwriter reuse,Varyheader interaction withwithNoStore, backwards compatibility for non-negotiating clients).config.Load,alert.NewNotifier,collector.New,web.Handler,httpapi.New). Dropped the count rather than correcting it to "five", per the issue's own suggestion — a count that must be kept in sync with the list is a trap that will just break again on the next change.Per the issue's own caveat, I re-verified all three claims against the current
main(not just trusting the issue body) before editing, since the issue was drafted by an AI review and the code may have moved since. Point 2 in particular had drifted further than the issue described (two more middlewares —withMaxInFlight,withNoStore— were added after the issue was filed), so the fix reflects the current chain, not the issue's literal suggested diff.This is a documentation-only change — no source files were touched.
🎫 Issues
Closes #115
👩💻 Reviewer Notes
Nothing to smoke-test; this is prose-only. Worth double-checking:
apiRouteininternal/httpapi/server.go.📑 Test Plan
Documentation-only change; per
docs/TESTS.mdno new Go tests are required or applicable. Ran the full existing suite to confirm nothing regressed:go build ./...— passesgo vet ./...— passesgo test ./... -race -cover— passes (all packages green)golangci-lint run— could not run in this environment (golangci-lint binary was built against an older Go toolchain than the repo'sgo.modtargets); no Go source was changed by this PR.✅ Checklist
General
go test ./... -race -coverpasses locally). (N/A — docs-only, no behavior changed)go vet ./...andgolangci-lint runare clean. (go vetclean;golangci-lintcould not run in this environment due to a toolchain version mismatch — no Go source changed)ARCHITECTURE.mdif this changes a documented design decision. (this PR's entire purpose)REST API / configuration / packaging
Not applicable — no API, config, or packaging changes.
⏭ Next Steps
None. The issue notes several other open issues touch the same
ARCHITECTURE.mdsections (API cache-header, request-throttling, asset-ETag, async-persistence); those are separate work and out of scope here.