Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot
AI
changed the title
Introduce a database boundary for dashboard call-table storage
Separate dashboard call-table storage from Redis operations
Oct 1, 2026
Copilot created this pull request from a session on behalf of
pelikhan
October 1, 2026 15:08
View session
pelikhan
marked this pull request as ready for review
October 1, 2026 15:13
Contributor
Dashboard query cost reportDashboard query cost (deployed SQLite snapshot)Measured the 10 most expensive of 164 queries chosen by the static query cost evaluator (normalized-upper-bound, 695 normalized row-read units to materialize all queries). Canonical projection read 42,194 records across 10 sources in 8813.16 ms. Canonical projection by source
Empty database tables: Computational cost
Space cost
Retained heap is measured after collection while the result is held; heap after run is a single post-execution sample, not an exact peak. Redis native translationRedis native translation (offline)185 queries: 9 full candidates, 2 partial candidates, 167 Go fallback, 7 unsupported by Go. Candidates are not verified native executions: the active Redis generation must have JSON sources and compatible RediSearch indexes. Partial candidates can still execute remaining operations in Go; no Redis connection or runtime row budget was checked.
Query cost benchmark passed. Redis translation report compiled. |
Contributor
Dashboard query parityStatus: passed All 160 dashboard queries matched across 7 backends. |
Contributor
Dashboard view assessmentNo dashboard views were potentially impacted by this pull request. |
Collaborator
|
@copilot database snapshoting is a redis internal implementation. Use a "ingest" function that takes a set of transactions and let the impl details out |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Some server reads bypass the new boundary, ingestion results can become inconsistent, and the architecture model omits the new relationships.
Review effort: Balanced
Findings: 2
Open (3)
What changed in this PR
Introduces a database abstraction between dashboard ingestion/querying and Redis-backed storage.
Changes:
- Adds snapshot, reader, validation, and execution contracts.
- Implements the contract using Redis generations.
- Routes ingestion and primary query paths through the abstraction with tests and documentation.
| File | Description |
|---|---|
ARCHITECTURE.md |
Documents the database boundary. |
CODEBASE.yml |
Adds the database component. |
server/README.md |
Explains storage responsibilities. |
server/internal/dashboarddb/database.go |
Defines storage interfaces and snapshot types. |
server/internal/ingest/database_test.go |
Tests generic snapshot publication and reuse. |
server/internal/ingest/ingest.go |
Publishes ingestion results through the database. |
server/internal/redisx/dashboard_database.go |
Implements the Redis adapter. |
server/internal/redisx/dashboard_database_test.go |
Tests snapshot pinning and validation. |
server/internal/server/canonical.go |
Routes canonical reads through the database. |
server/internal/server/server.go |
Routes dashboard queries through pinned readers. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+135
to
+139
| if err == nil { | ||
| active, activeErr := store.Active(ctx) | ||
| if activeErr == nil { | ||
| result.Generation = active.Generation | ||
| } |
|
|
||
| type App struct { | ||
| store *redisx.Store | ||
| dashboard dashboarddb.Database |
Comment on lines
+141
to
+143
| dashboard_database: | ||
| path: server/internal/dashboarddb/ | ||
| responsibility: expose generation-free snapshot publication, database state, query validation, and query execution for dashboard call-table sources; Redis-specific generation and index management belongs to the Redis adapter, while operational caches and state remain separate |
Collaborator
|
@copilot database snapshoting is a redis internal implementation. Use a "ingest" function that takes a set of transactions and let the impl details out |
Contributor
Dashboard query performanceQuery performance measurementsPopulation time: 69198.20 ms IndexedDB count()Counted 362,374 records across 18 stores in 497.50 ms. Overview critical pathInitial Overview ready: 3424.70 ms Settled deployed-data query: 812.80 ms (23,111 records read)
Slowest standalone Overview sources:
All dashboard queriesChunk size: 25 rows
|
Contributor
|
Go Redis dashboard integration: PASSED Per-page resultsGo Redis dashboard page checksPASSED — 57/57 pages passed.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Changed the database boundary to |
Contributor
Author
This request is already implemented: the boundary exposes |
pelikhan
reviewed
Oct 1, 2026
| active, err := service.store.Active(ctx) | ||
| database := service.dashboard | ||
| if database == nil { | ||
| database = &redisx.DashboardDatabase{Store: service.store} |
Collaborator
There was a problem hiding this comment.
@copilot avoid pollution of concepts here. Service should have database field pre populated.
Contributor
Author
There was a problem hiding this comment.
Updated canonicalService to require its database dependency explicitly, removed its lazy Redis adapter construction, and initialized one shared adapter in New. Commit: bfff24b.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Added an accepted ADR documenting the database boundary, why Redis generation and indexing remain adapter details, reader consistency, alternatives, and consequences. Commit: |
Collaborator
|
@copilot make adr self contained with description of the database interface |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Expanded the ADR with the complete |
Collaborator
|
/matt review design abstraction, clean up and prepare for Postgres migration |
Contributor
|
✅ Matt Pocock Skills Reviewer completed successfully!
|
pelikhan
reviewed
Oct 1, 2026
| type Database interface { | ||
| Current(context.Context) (Reader, error) | ||
| Ingest(context.Context, Transactions) (State, error) | ||
| Validate([]query.Definition) error |
Contributor
Author
There was a problem hiding this comment.
Added context.Context to Database.Validate, passed the request context through, and made the Redis adapter return pre-canceled context errors. Added a cancellation test and updated the ADR. Commit: 1c9f822.
Collaborator
|
@copilot issue state updates must also be intergrate in the abstraction layer. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
codebase-design review
Applied skills: codebase-design (database boundary/seam correctness) and tdd (coverage of the new contract).
This PR establishes a clean dashboarddb.Database/Reader seam and the Redis adapter implements it well, with the canonical.go pollution issue from @pelikhan's earlier comment already resolved in bfff24b (service now takes database as a required constructor dependency, no lazy adapter). Good test coverage of the new boundary in database_test.go and dashboard_database_test.go.
However, three defects already flagged by the automated Copilot review remain unresolved in the current diff and block merge:
-
server/internal/ingest/ingest.go(around line 139):Runsilently swallowsstore.Activeerrors when populating the legacyGenerationfield, which can attach a later generation's name to an earlier result'sRevision/DataRevision/Counts. This breaks the newReaderconsistency guarantee this PR's ADR explicitly promises (pinned revision/response agreement). -
server/internal/server/server.go(around line 81 / theAppstruct and/api/v1/refreshhandler): The dashboard database is not yet the sole source of truth —refresh, diagnostics, and repository-memory handlers still reada.store.Active/redisx.Storedirectly instead of going througha.dashboard. This leaves two divergent read paths for the same generation state, undermining the boundary this PR's ADR claims to establish. -
CODEBASE.yml(around line 143):dashboard_databaseis declared as a component but never appears independencies; every server flow still modelsdashboard_https_server -> redis_projectiondirectly, so the architectural boundary isn't reflected in the dependency graph consumers rely on. Adddashboard_https_server -> dashboard_database(anddashboard_database -> redis_projectionif the adapter relationship should be explicit) edges.
None of these are addressed by the latest commits (08d7206, 055b363, bfff24b). Please resolve items 1–3 (or explain why they're out of scope) before merge — items 1 and 2 are correctness/consistency defects that directly contradict the stated goal of pinning queries to one snapshot and making the database the sole source of truth.
Summary table
| # | Status | File | Concern |
|---|---|---|---|
| 1 | Open (pre-existing finding) | server/internal/ingest/ingest.go |
Silently ignored Active error can misattach generation/result data |
| 2 | Open (pre-existing finding) | server/internal/server/server.go |
Refresh/diagnostics/memory handlers bypass the new database boundary |
| 3 | Open (pre-existing finding) | CODEBASE.yml |
New dashboard_database component missing from dependencies graph |
| 4 | Resolved | server/internal/server/canonical.go |
Lazy adapter construction removed per bfff24b; database now required |
Generated by Matt Pocock Skills Reviewer · copilot · auto · 54.8 AIC · ⌖ 5.65 AIC · ⊞ 7.3K
Comment /matt to run again
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Added |
Collaborator
Contributor
Author
Plan for integrating the abstraction into #15576:
|
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
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.
Dashboard call-table storage is currently coupled to Redis generation and projection mechanics. This PR introduces a database boundary for ingestion and queries without adding a PostgreSQL backend.