chore: psql changelog #702
Conversation
WalkthroughIntroduces a comprehensive changelog persistence layer with PostgreSQL backing, including a Go-based Store implementation for upserting and deleting changelog entries, Drizzle ORM schema definition, SQL migration, and extensive test coverage. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant Store
participant DB as PostgreSQL
participant Tx as Transaction
Caller->>Store: Save(ctx, changes)
Store->>Store: separateChangeTypes(changes)
Note over Store: Splits into upserts & deletes
Store->>Tx: BEGIN
activate Tx
Tx->>DB: batchUpsertChangelogEntries
Note over DB: INSERT ... ON CONFLICT DO UPDATE
DB-->>Tx: ✓
Tx->>DB: batchDeleteChangelogEntries
Note over DB: DELETE by (workspace_id, entity_type, entity_id)
DB-->>Tx: ✓
Tx->>DB: COMMIT
deactivate Tx
DB-->>Store: Success or Error
Store-->>Caller: error
Caller->>Store: Load(ctx, namespace)
Store->>DB: Query changelog_entry by workspace_id
DB-->>Store: rows
Store->>Store: unmarshalEntity(entityType, data)
Note over Store: Deserialize 15+ entity types
Store->>Store: Build persistence.Changes
Store-->>Caller: persistence.Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📊 DB Package Test Coveragepkg/db coverage: |
📊 Code Coverage Reportworkspace-engine coverage: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
packages/db/src/schema/changelog_entry.ts (1)
12-26: Schema matches migration; solid baseline.
- Composite PK and ON DELETE CASCADE are appropriate.
- Optional: narrow entityData typing (unknown or a zod-backed union) and expose a zod schema for validation.
- Optional: if reads often sort by recency per workspace, consider an index on (workspace_id, created_at DESC); PK helps equality, but not ordered scans.
packages/db/drizzle/0134_nappy_pretty_boy.sql (1)
1-10: DDL aligns with the TypeScript schema.
- Looks correct.
- Optional: add an index to support common reads, e.g.:
CREATE INDEX IF NOT EXISTS idx_changelog_entry_workspace_created_at ON changelog_entry (workspace_id, created_at DESC);- Optional: enforce non-empty entity_type:
ALTER TABLE changelog_entry ADD CONSTRAINT entity_type_nonempty CHECK (length(entity_type) > 0);apps/workspace-engine/pkg/db/persistence/store.go (3)
94-121: Batch size can exceed Postgres parameter limits.For large batches you may hit 65k bind parameters (5/row upsert, 3/row delete). Chunk changes accordingly (e.g., max ~10k for upsert). Optional but prevents rare failures.
163-170: Return a typed “not implemented” error from Load.Returning (nil, nil) can mask accidental use.
func (s *Store) Load(ctx context.Context, namespace string) (persistence.Changes, error) { - return nil, nil + return nil, fmt.Errorf("Load not implemented") }
16-26: Connection ownership and concurrency.Store holds a single pgxpool.Conn. It’s not goroutine‑safe to use concurrently. Prefer holding the pool and Acquire/Release per Save, or document single‑goroutine use.
apps/workspace-engine/pkg/db/persistence/store_test.go (1)
400-454: Add “last write wins” batch tests.Please add cases where the same entity appears twice in a single Save:
- set → unset: expect deletion
- unset → set: expect presence with latest data
These will guard against ordering regressions in Save.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
apps/workspace-engine/pkg/db/persistence/store.go(1 hunks)apps/workspace-engine/pkg/db/persistence/store_test.go(1 hunks)packages/db/drizzle/0134_nappy_pretty_boy.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/changelog_entry.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/workspace-engine/**/*.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
apps/workspace-engine/**/*.go: Do not add extraneous inline comments that state the obvious
Do not add comments that simply restate what the code does
Do not add comments for standard Go patterns (e.g., noting WaitGroup or semaphore usage)
Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods
Files:
apps/workspace-engine/pkg/db/persistence/store.goapps/workspace-engine/pkg/db/persistence/store_test.go
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript with explicit types (prefer interfaces for public APIs)
Import styles: Use named imports, group imports by source (std lib > external > internal)
Consistent type imports:import type { Type } from "module"
Prefer async/await over raw promises
Handle errors explicitly (use try/catch and typed error responses)
Files:
packages/db/src/schema/changelog_entry.ts
⚙️ CodeRabbit configuration file
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
Files:
packages/db/src/schema/changelog_entry.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
Formatting: Prettier is used with
@ctrlplane/prettier-config
Files:
packages/db/src/schema/changelog_entry.tspackages/db/drizzle/meta/_journal.json
apps/workspace-engine/**/*_test.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
Follow the existing test structure used in *_test.go files
Files:
apps/workspace-engine/pkg/db/persistence/store_test.go
🧠 Learnings (1)
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
PR: ctrlplanedev/ctrlplane#0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/**/*_test.go : Follow the existing test structure used in *_test.go files
Applied to files:
apps/workspace-engine/pkg/db/persistence/store_test.go
🧬 Code graph analysis (3)
apps/workspace-engine/pkg/db/persistence/store.go (2)
apps/workspace-engine/pkg/db/client.go (2)
GetDB(60-70)Close(73-77)apps/workspace-engine/pkg/persistence/changelog.go (4)
Changes(34-34)ChangeTypeSet(19-19)ChangeTypeUnset(20-20)Entity(9-13)
packages/db/src/schema/changelog_entry.ts (1)
packages/db/src/schema/workspace.ts (1)
workspace(19-28)
apps/workspace-engine/pkg/db/persistence/store_test.go (3)
apps/workspace-engine/pkg/db/client.go (3)
Close(73-77)GetPool(30-57)GetDB(60-70)apps/workspace-engine/pkg/db/persistence/store.go (1)
Store(16-18)apps/workspace-engine/pkg/persistence/changelog.go (4)
Entity(9-13)Changes(34-34)ChangeTypeSet(19-19)ChangeTypeUnset(20-20)
🔇 Additional comments (1)
packages/db/drizzle/meta/_journal.json (1)
943-949: Journal entry consistency looks good.idx, tag, and version align with the new migration. Please run the usual drizzle verify before merge.
| func TestStore_Close(t *testing.T) { | ||
| _, _, conn := setupTestWithWorkspace(t) | ||
| store := createTestStore(t, conn) | ||
|
|
||
| // Close should not error | ||
| err := store.Close() | ||
| if err != nil { | ||
| t.Fatalf("Close failed: %v", err) | ||
| } |
There was a problem hiding this comment.
Double Release of pgxpool.Conn in TestStore_Close.
setupTestWithWorkspace schedules conn.Release() via t.Cleanup, and store.Close() releases it again → potential panic. Acquire a fresh conn just for this test or skip t.Cleanup for that conn.
Apply one of these diffs:
Option A — acquire directly and don’t register cleanup:
func TestStore_Close(t *testing.T) {
- _, _, conn := setupTestWithWorkspace(t)
- store := createTestStore(t, conn)
+ ctx := context.Background()
+ conn, err := db.GetDB(ctx)
+ if err != nil {
+ t.Fatalf("Failed to get DB connection: %v", err)
+ }
+ store := createTestStore(t, conn)
// Close should not error
- err := store.Close()
+ err = store.Close()
if err != nil {
t.Fatalf("Close failed: %v", err)
}
}Option B — keep setup helper but don’t call store.Close(); instead, verify Close is idempotent by constructing Store via NewStore so ownership is clear.
Committable suggestion skipped: line range outside the PR's diff.
| func (s *Store) separateChangeTypes(changes persistence.Changes) (persistence.Changes, persistence.Changes) { | ||
| var upsertChanges persistence.Changes | ||
| var deleteChanges persistence.Changes | ||
| for _, change := range changes { | ||
| if change.ChangeType == persistence.ChangeTypeSet { | ||
| upsertChanges = append(upsertChanges, change) | ||
| } | ||
|
|
||
| if change.ChangeType == persistence.ChangeTypeUnset { | ||
| deleteChanges = append(deleteChanges, change) | ||
| } | ||
| } | ||
| return upsertChanges, deleteChanges | ||
| } |
There was a problem hiding this comment.
Reordering breaks “last write wins” semantics.
Splitting into upserts then deletes changes intent-order: if the same entity appears multiple times in a batch, delete always wins even when the last change is a set. This contradicts the compaction contract (“keep only the latest”). Fix by collapsing to the last change per (namespace, entityType, entityId) before batching.
Apply this diff to make separateChangeTypes perform last‑wins compaction:
func (s *Store) separateChangeTypes(changes persistence.Changes) (persistence.Changes, persistence.Changes) {
- var upsertChanges persistence.Changes
- var deleteChanges persistence.Changes
- for _, change := range changes {
- if change.ChangeType == persistence.ChangeTypeSet {
- upsertChanges = append(upsertChanges, change)
- }
-
- if change.ChangeType == persistence.ChangeTypeUnset {
- deleteChanges = append(deleteChanges, change)
- }
- }
- return upsertChanges, deleteChanges
+ if len(changes) == 0 {
+ return nil, nil
+ }
+ type key struct{ ns, et, id string }
+ last := make(map[key]persistence.Change, len(changes))
+ for _, ch := range changes {
+ et, id := ch.Entity.CompactionKey()
+ last[key{ns: ch.Namespace, et: et, id: id}] = ch // last write wins
+ }
+ var upsertChanges, deleteChanges persistence.Changes
+ for _, ch := range last {
+ switch ch.ChangeType {
+ case persistence.ChangeTypeSet:
+ upsertChanges = append(upsertChanges, ch)
+ case persistence.ChangeTypeUnset:
+ deleteChanges = append(deleteChanges, ch)
+ }
+ }
+ return upsertChanges, deleteChanges
}Also add tests covering “set→unset” and “unset→set” within one Save call (see test comment).
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/db/persistence/store.go around lines 28 to 41, the
current separateChangeTypes groups changes by type which breaks last-write-wins;
replace it with a last-wins compaction: build a map keyed by
namespace|entityType|entityId and iterate the incoming changes in order, storing
each change into the map so later entries overwrite earlier ones, then iterate
the map values and split them into upsert (ChangeTypeSet) and delete
(ChangeTypeUnset) slices to return. Also add unit tests for Save covering both
set→unset and unset→set within a single call to assert the last change wins.
| // Use CopyFrom to efficiently batch upsert, but for simplicity, using INSERT ... ON CONFLICT | ||
| sql := ` | ||
| INSERT INTO changelog_entry | ||
| (workspace_id, entity_type, entity_id, entity_data, created_at) | ||
| VALUES | ||
| %s | ||
| ON CONFLICT (workspace_id, entity_type, entity_id) | ||
| DO UPDATE SET | ||
| entity_data = EXCLUDED.entity_data, | ||
| created_at = EXCLUDED.created_at | ||
| ` | ||
| // Prepare the value placeholders and arguments | ||
| valueStrings := make([]string, 0, len(changes)) | ||
| valueArgs := make([]any, 0, len(changes)*5) | ||
|
|
||
| argIdx := 1 | ||
| for _, change := range changes { | ||
| // Assumes change.Namespace is workspace_id, change.Entity.CompactionKey() -> entity_type, entity_id | ||
| entityType, entityID := change.Entity.CompactionKey() | ||
| valueStrings = append(valueStrings, | ||
| fmt.Sprintf("($%d, $%d, $%d, $%d, $%d)", argIdx, argIdx+1, argIdx+2, argIdx+3, argIdx+4), | ||
| ) | ||
| entityData, err := json.Marshal(change.Entity) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| valueArgs = append(valueArgs, | ||
| change.Namespace, | ||
| entityType, | ||
| entityID, | ||
| entityData, | ||
| change.Timestamp, | ||
| ) | ||
| argIdx += 5 | ||
| } | ||
| query := fmt.Sprintf(sql, joinValueStrings(valueStrings)) | ||
|
|
||
| _, err := tx.Exec(ctx, query, valueArgs...) | ||
| return err | ||
| } |
There was a problem hiding this comment.
Guard zero timestamps; minor ergonomics.
- If change.Timestamp is zero, you’ll write an ancient timestamp. Fallback to time.Now() before appending args.
- Minor: use strings.Join for valueStrings to simplify joinValueStrings.
-import (
+import (
"context"
"encoding/json"
"fmt"
+ "time"
"workspace-engine/pkg/db"
"workspace-engine/pkg/persistence"- valueArgs = append(valueArgs,
+ ts := change.Timestamp
+ if ts.IsZero() {
+ ts = time.Now()
+ }
+ valueArgs = append(valueArgs,
change.Namespace,
entityType,
entityID,
entityData,
- change.Timestamp,
+ ts,
)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/db/persistence/store.go around lines 48 to 87,
guard against zero change.Timestamp by computing ts := change.Timestamp; if
ts.IsZero() { ts = time.Now() } and append ts (not change.Timestamp) to
valueArgs so you don’t write a zero/ancient time; also replace the custom
joinValueStrings call with strings.Join(valueStrings, ",") and add imports for
"time" and "strings" as needed.
adityachoudhari26
force-pushed
the
psql-changelog
branch
from
7eaa922 to
7af3d16
Compare
📊 DB Package Test Coveragepkg/db coverage: |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
apps/workspace-engine/pkg/db/persistence/store_test.go (1)
543-551: Double release issue already flagged.The past review comment correctly identified that this test causes a double release of the connection (once via
t.CleanupinsetupTestWithWorkspaceand once viastore.Close()).apps/workspace-engine/pkg/db/persistence/store.go (3)
30-43: Last-write-wins semantics issue already flagged.The past review comment correctly identified that this implementation breaks last-write-wins semantics by not deduplicating entities with the same compaction key before separating by type.
45-89: Zero timestamp handling already flagged; optional optimization available.The past review comment correctly identified the zero timestamp issue on Line 81. Additionally, Line 50's comment suggests using
CopyFromfor better performance, which could be a future optimization for large batches.
126-135: String concatenation optimization already suggested.The past review comment suggested using
strings.Joininstead of manual concatenation for better readability.
🧹 Nitpick comments (1)
apps/workspace-engine/pkg/db/persistence/store_test.go (1)
42-56: Consider documenting database requirements.The hardcoded connection string on Line 42 assumes a specific local PostgreSQL setup. While the environment variable fallback on Line 46 provides flexibility, consider adding a comment explaining the database requirements for running these tests.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
apps/workspace-engine/pkg/db/persistence/store.go(1 hunks)apps/workspace-engine/pkg/db/persistence/store_test.go(1 hunks)packages/db/drizzle/0134_lowly_lady_deathstrike.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/changelog_entry.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/src/schema/changelog_entry.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/workspace-engine/**/*.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
apps/workspace-engine/**/*.go: Do not add extraneous inline comments that state the obvious
Do not add comments that simply restate what the code does
Do not add comments for standard Go patterns (e.g., noting WaitGroup or semaphore usage)
Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods
Files:
apps/workspace-engine/pkg/db/persistence/store_test.goapps/workspace-engine/pkg/db/persistence/store.go
apps/workspace-engine/**/*_test.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
Follow the existing test structure used in *_test.go files
Files:
apps/workspace-engine/pkg/db/persistence/store_test.go
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
Formatting: Prettier is used with
@ctrlplane/prettier-config
Files:
packages/db/drizzle/meta/_journal.json
🧬 Code graph analysis (2)
apps/workspace-engine/pkg/db/persistence/store_test.go (4)
apps/workspace-engine/pkg/oapi/oapi.gen.go (2)
Status(57-57)Pending(41-41)apps/workspace-engine/pkg/db/client.go (3)
Close(73-77)GetPool(30-57)GetDB(60-70)apps/workspace-engine/pkg/db/persistence/store.go (1)
Store(18-20)apps/workspace-engine/pkg/persistence/changelog.go (4)
Entity(9-13)Changes(34-34)ChangeTypeSet(19-19)ChangeTypeUnset(20-20)
apps/workspace-engine/pkg/db/persistence/store.go (2)
apps/workspace-engine/pkg/db/client.go (2)
GetDB(60-70)Close(73-77)apps/workspace-engine/pkg/persistence/changelog.go (4)
Changes(34-34)ChangeTypeSet(19-19)ChangeTypeUnset(20-20)Entity(9-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: workspace-engine-tests
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: workspace-engine-tests
- GitHub Check: build-and-push (linux/amd64)
🔇 Additional comments (14)
packages/db/drizzle/0134_lowly_lady_deathstrike.sql (1)
1-10: LGTM! Clean migration structure.The migration correctly creates the
changelog_entrytable with an appropriate composite primary key for workspace isolation and a cascade delete foreign key constraint.packages/db/drizzle/meta/_journal.json (1)
943-949: LGTM! Journal entry correctly added.The new migration entry is properly sequenced and formatted.
apps/workspace-engine/pkg/db/persistence/store_test.go (6)
18-38: LGTM! Clean mock implementations.The mock entity types correctly implement the
CompactionKey()interface for testing.
59-111: LGTM! Proper resource management.The helper correctly manages connections and ensures cleanup via workspace deletion with cascade.
114-170: LGTM! Well-structured test helpers.The helper functions provide clean abstractions for common test operations.
174-541: LGTM! Comprehensive Save operation coverage.The tests thoroughly cover upsert, delete, mixed operations, workspace isolation, and transaction rollback scenarios.
556-973: LGTM! Thorough Load operation coverage.The tests comprehensively verify Load functionality across various scenarios including empty workspaces, multiple entity types, updates, deletions, and workspace isolation.
977-1455: LGTM! Excellent OAPI entity type coverage.The tests comprehensively verify all supported OAPI entity types individually and together, ensuring robust serialization/deserialization.
apps/workspace-engine/pkg/db/persistence/store.go (6)
16-28: LGTM! Clean constructor pattern.The Store initialization properly handles connection acquisition errors.
91-123: LGTM! Safe parameterized deletion.The batch delete correctly uses parameterized queries with the composite primary key.
137-163: LGTM! Proper transaction handling.The Save method correctly manages the transaction lifecycle with appropriate error wrapping.
165-209: LGTM! Clean Load implementation.The method properly queries, iterates, and unmarshals entities with appropriate error handling.
212-322: LGTM! Comprehensive entity type handling.The switch statement explicitly handles all 15 supported entity types with consistent error handling for unknown types. While this could potentially be refactored using reflection or a registry pattern, the explicit approach provides clarity and type safety.
324-327: LGTM! Proper resource cleanup.The Close method correctly releases the connection.
1 participant
Summary by CodeRabbit
New Features
Tests