refactor: architecture deepening — 9 review candidates across web and backend - #78
Merged
Merged
Conversation
…ics response type
…meters The dashboard route hardcoded request-evidence pageSize=1 while the load plan emitted 10 — the tested value was never wired. git a226bee shows page_size=1 is deliberate ('latest evidence pagination': the panel only renders events[0] as the latest request), so the plan now emits 1 and the route consumes fixedWindow.requestEvidence.pageSize instead of a literal. No runtime behavior change. Also narrow useAnalyticsCore/useAnalyticsHeatmap to non-null TimeGranularity and drop the unreachable in-hook getDefaultGranularity fallback; the only caller (the dashboard route) always passes a resolved granularity.
…ntity lookup seam
…l reads - NewRouter takes a plain OptionalProviders struct instead of variadic-as-optional - AuthConfig built once in app.NewWithConfig and shared by handler and router - usageIdentityService/rollupBackfillService shallow wrappers deleted; handlers consume repository.UsageIdentityReader/RollupBackfillReader through api seams (aligns with ADR-0005 read-path amendment) - RollupBackfill status read now honors ctx end to end (reader seam + GetUsageRollupBackfillStatus WithContext); backfill batch intentionally keeps an internal background context since cancellation only applies between batches
- move HasActiveIdentity existence check into repository (layout ownership) - thread ctx through canonical event lookup repository functions (seam honesty, consistent with the analytics/usage reader ctx threading) - unify canonical event key building behind one BuildEventKey call path - drop unused ListUsageIdentities stub method from api tests
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
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
1 participant
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.
Summary
Architecture deepening pass from the 2026-07-30 architecture review: 9 candidates landed, one commit each, plus a code-review fix commit. All changes are pure refactors behind existing seams — no HTTP contract or product-observable behavior change (one disclosed exception below).
Frontend (
web/):useAnalyticssummary variant,mergeAnalyticsCore, and the wideAnalyticsResponsetype — all had zero production callers (2469c0d).routes/index.lazy.tsxinto testedfeatures/usage-intelligenceview-models (surfaces.ts,refresh.ts); the route is now pure composition (+17 Vitest cases) (d3755c4).pageSize=1is the real contract (backend explicitly accepts it for "latest evidence") and the plan's10was the fiction — the plan was corrected instead of the route; hook signatures narrowed to non-nullTimeGranularity(7c31786).features/usage-intelligencewith interfaces keyed on Time Granularity instead ofrange; components are now thin render adapters (+29 Vitest cases) (2ce20a6).Backend (
internal/):quota.Serviceinto a task store, register worker lifecycle with the app background-task set (shutdown no longer orphans in-flight tasks), inject a narrowAuthFileIdentityLookupinstead of*gorm.DB— quota dispatch tests no longer need sqlite; deadRefreshSourcevocabulary removed (259d253).SyncNow/SyncOnce/SyncStatus/SyncRedisBatch/NewSyncServiceWithClient/CleanupRedisUsageInbox), turn the provider-metadata fan-out into an ordered registry (mirroring the quota registry), and eliminate the hidden mutation insidevalidate()(1e44674).CanonicalEventKeyAssignerwith a narrowCanonicalEventLookupseam; the full branch matrix is now table-driven without sqlite (0e72c11).context.ContextfromUsageReader/AnalyticsReaderthroughBuild*WithFilterto GORMWithContext— expensive analytics queries can now be cancelled on client disconnect, and cancellation is asserted through the seam (6bdb933).OptionalProvidersis a plain struct (no variadic-as-optional),AuthConfigis built once, and the shallowusageIdentityService/rollupBackfillServicewrappers are folded intorepositoryreaders consumed through theinternal/apiseams, per the ADR-0005 amendment (2e1446b).HasActiveIdentitymoved intointernal/repository, canonical lookup functions honor ctx, canonical key building unified behind oneBuildEventKeypath (41ed28d).Verification
make verifyexit 0, run twice (after the 9 candidate commits and again after the review-fix commit): backendgo test+go vet, frontend lint + Vitest (105 tests) + typecheck, Playwright e2e 12/12.6fa5c6e: Spec axis confirmed all 9 candidates present with line-identical semantics and ADR-0002~0005 boundaries held; Standards axis findings fixed in41ed28d.Compatibility
RefreshSourcewas never sent or consumed over HTTP.Auto-close: none