chore: write everything to db from changeset#684
Conversation
WalkthroughIntroduces per-workspace changeset processing by threading workspaceID through FlushChangeset/applyChange, expands entity handling to include UserApprovalRecord, Release, and Job operations, implements real Release deletion, re-enables UserApprovalRecord upsert/delete logic, wires DbChangesetConsumer into workspace.New(), and adds unit tests for UserApprovalRecord CRUD/upsert. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller as Workspace.New(...)
participant WS as Workspace
participant Cons as DbChangesetConsumer (workspaceID)
participant DB as DB (pgx.Tx)
participant Handler as applyChange
participant Entities as Entity Writers/Deleters
Caller->>WS: New(id)
WS->>Cons: db.NewChangesetConsumer(id)
note right of Cons: Consumer stores workspaceID
Caller->>Cons: FlushChangeset(ctx, cs)
Cons->>DB: Begin Tx
loop for each change
Cons->>Handler: applyChange(ctx, tx, change, workspaceID)
alt UserApprovalRecord
Handler->>Entities: write/deleteUserApprovalRecord(tx)
else Release
Handler->>Entities: writeRelease(ctx, workspaceID, tx) / deleteRelease(ctx, releaseID, tx)
else Job
Handler->>Entities: writeJob(tx) / deleteJob(tx)
else Other entities
Handler->>Entities: existing handlers...
end
end
Handler-->>Cons: per-change result
Cons->>DB: Commit / Rollback on error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/workspace-engine/pkg/workspace/workspace.go (1)
101-116: Reinitialize changesetConsumer after GobDecodeAfter decoding the store and rebuilding releasemanager, changesetConsumer remains nil. Re‑set it to a workspace‑scoped consumer to avoid nil use post-restore.
func (w *Workspace) GobDecode(data []byte) error { // Initialize store if needed if w.store == nil { w.store = &store.Store{} } // Decode the store if err := w.store.GobDecode(data); err != nil { return err } // Reinitialize release manager with the decoded store w.releasemanager = releasemanager.New(w.store) + // Reinitialize DB changeset consumer for this workspace + w.changesetConsumer = db.NewChangesetConsumer(w.ID) return nil }apps/workspace-engine/pkg/db/user_approval_records.go (2)
59-67: Upsert sets approved_at from EXCLUDED without providing itEXCLUDED.approved_at isn’t supplied by the insert list; it will default (likely NULL). If you intend to stamp approval time, set it conditionally, e.g., when status='approved', and preserve prior value otherwise.
-ON CONFLICT (user_id, deployment_version_id, environment_id) DO UPDATE SET - status = EXCLUDED.status, - reason = EXCLUDED.reason, - approved_at = EXCLUDED.approved_at, - updated_at = NOW() +ON CONFLICT (user_id, deployment_version_id, environment_id) DO UPDATE SET + status = EXCLUDED.status, + reason = EXCLUDED.reason, + approved_at = CASE + WHEN EXCLUDED.status = 'approved' THEN COALESCE(EXCLUDED.approved_at, NOW()) + ELSE NULL + END, + updated_at = NOW()
69-75: Pass CreatedAt as time.Time instead of stringAvoid relying on server‑side text→timestamp casts. Parse RFC3339 into time.Time and pass that to Exec.
-_, err := tx.Exec(ctx, USER_APPROVAL_RECORD_UPSERT_QUERY, userApprovalRecord.UserId, userApprovalRecord.VersionId, userApprovalRecord.EnvironmentId, userApprovalRecord.Status, userApprovalRecord.Reason, userApprovalRecord.CreatedAt) +ts, perr := time.Parse(time.RFC3339, userApprovalRecord.CreatedAt) +if perr != nil { + return perr +} +_, err := tx.Exec(ctx, USER_APPROVAL_RECORD_UPSERT_QUERY, + userApprovalRecord.UserId, + userApprovalRecord.VersionId, + userApprovalRecord.EnvironmentId, + userApprovalRecord.Status, + userApprovalRecord.Reason, + ts, +)Note: add
timeimport.apps/workspace-engine/pkg/db/user_approval_records_test.go (1)
600-1191: Solid coverage; consider extracting setup helpersTests comprehensively cover read scope, write/upsert, nil reason, deletes, and workspace isolation. To reduce duplication, factor repeated “create dependencies” code into helpers.
📜 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/changeset.go(4 hunks)apps/workspace-engine/pkg/db/releases.go(1 hunks)apps/workspace-engine/pkg/db/user_approval_records.go(1 hunks)apps/workspace-engine/pkg/db/user_approval_records_test.go(1 hunks)apps/workspace-engine/pkg/workspace/workspace.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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/releases.goapps/workspace-engine/pkg/workspace/workspace.goapps/workspace-engine/pkg/db/user_approval_records_test.goapps/workspace-engine/pkg/db/user_approval_records.goapps/workspace-engine/pkg/db/changeset.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/user_approval_records_test.go
🧬 Code graph analysis (4)
apps/workspace-engine/pkg/workspace/workspace.go (1)
apps/workspace-engine/pkg/db/changeset.go (1)
NewChangesetConsumer(140-144)
apps/workspace-engine/pkg/db/user_approval_records_test.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (3)
UserApprovalRecord(339-346)ApprovalStatusApproved(19-19)ApprovalStatusRejected(20-20)
apps/workspace-engine/pkg/db/user_approval_records.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
UserApprovalRecord(339-346)
apps/workspace-engine/pkg/db/changeset.go (3)
apps/workspace-engine/pkg/changeset/changeset.go (2)
ChangeSet(23-29)Change(17-21)apps/workspace-engine/pkg/oapi/oapi.gen.go (3)
UserApprovalRecord(339-346)Release(269-275)Job(148-159)apps/workspace-engine/pkg/changeset/changeset_consumer.go (1)
ChangesetConsumer(7-9)
⏰ 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). (4)
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: workspace-engine-tests
- GitHub Check: build (linux/amd64)
🔇 Additional comments (4)
apps/workspace-engine/pkg/workspace/workspace.go (1)
7-7: Per‑workspace ChangesetConsumer wiring — LGTMCorrectly scopes the consumer to the workspace via db.NewChangesetConsumer(id).
Also applies to: 18-18
apps/workspace-engine/pkg/db/user_approval_records.go (1)
77-87: Delete path — LGTMStraightforward transactional delete by composite key. Idempotent behavior is acceptable.
apps/workspace-engine/pkg/db/changeset.go (2)
12-44: Per‑workspace FlushChangeset + transaction handling — LGTMThreading workspaceID and clearing changes post‑commit looks correct.
134-148: DbChangesetConsumer with workspace scope — LGTMConstructor + method delegate correctly carry workspaceID into processing.
| if e, ok := change.Entity.(*oapi.UserApprovalRecord); ok && e != nil { | ||
| if change.Type == changeset.ChangeTypeDelete { | ||
| return deleteUserApprovalRecord(ctx, e, conn) | ||
| } | ||
| return writeUserApprovalRecord(ctx, e, conn) | ||
| } | ||
|
|
||
| if e, ok := change.Entity.(*oapi.Release); ok && e != nil { | ||
| if change.Type == changeset.ChangeTypeDelete { | ||
| return deleteRelease(ctx, e.ID(), conn) | ||
| } | ||
| return writeRelease(ctx, e, workspaceID, conn) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Release delete calls e.ID() — verify this API exists
Ensure oapi.Release defines ID() string. If not, this won’t compile. If release ID isn’t directly available, pass the correct identifier or derive it consistently.
Run to confirm:
🏁 Script executed:
#!/bin/bash
# Does Release expose ID()?
rg -nP -C2 'type\s+Release\b' apps | sed -n '1,120p'
rg -nP -C2 'func\s*\(\s*\*?Release\)\s+ID\s*\('
# Find all direct invocations of FlushChangeset to ensure new signature usages are updated
rg -nP -C3 '\bFlushChangeset\s*\('Length of output: 4608
Replace e.ID() with field access on Release
oapi.Release doesn’t define an ID() method; it has an ID field. Change deleteRelease(ctx, e.ID(), conn) to deleteRelease(ctx, e.ID, conn) or add an accessor if needed.
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/db/changeset.go around lines 110 to 122, the code
calls a non-existent method e.ID() on *oapi.Release; replace the call with the
struct field access e.ID (i.e., change deleteRelease(ctx, e.ID(), conn) to
deleteRelease(ctx, e.ID, conn)) or, alternatively, add a proper ID() accessor on
oapi.Release if the project convention requires a method — implement the change
accordingly and run `go vet`/`go build` to verify.
| const DELETE_RELEASE_QUERY = ` | ||
| DELETE FROM release WHERE id = $1 | ||
| ` | ||
|
|
||
| func deleteRelease(ctx context.Context, releaseID string, tx pgx.Tx) error { | ||
| _, err := tx.Exec(ctx, DELETE_RELEASE_QUERY, releaseID) | ||
| return err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Release delete implementation — verify workspace isolation and FK cleanup
The transactional delete is good. Two checks:
- Ensure FK cascade (jobs, version_release, variable_set_release(+_value), etc.) prevents orphans. If not, extend delete to clean dependents.
- Consider guarding by workspace to avoid cross‑workspace deletion if a foreign releaseID is supplied.
You can verify both with:
🏁 Script executed:
#!/bin/bash
# 1) Show FK constraints referencing "release"
rg -nP -C2 '(REFERENCES\s+release\b|FOREIGN KEY.*\brelease\b)' --type=sql
# 2) Look for ON DELETE CASCADE mentions
rg -nP -C2 'ON\s+DELETE\s+CASCADE' --type=sql
# 3) Confirm all callsites of deleteRelease and whether workspaceID is available
rg -nP -C2 '\bdeleteRelease\s*\('
# 4) If desired, consider a workspace-scoped delete query:
# DELETE FROM release r
# USING version_release vr, variable_set_release vsr, release_target rt, resource res
# WHERE r.id=$1 AND r.version_release_id=vr.id AND r.variable_release_id=vsr.id
# AND vsr.release_target_id=rt.id AND rt.resource_id=res.id AND res.workspace_id=$2;Length of output: 6859
🏁 Script executed:
#!/bin/bash
rg -nP 'CREATE TABLE\s+"release"' -g 'packages/db/drizzle/*.sql' -C5
rg -nP 'workspace_id' -g 'packages/db/drizzle/*.sql' -C2Length of output: 37169
🏁 Script executed:
#!/bin/bash
# Find release table creation more broadly
rg -nP '(CREATE TABLE.*"release"|ALTER TABLE "release")' -g 'packages/db/drizzle/*.sql' -A8 -B2 | head -100Length of output: 10279
Enforce workspace-scoped deleteRelease
FK constraints cover dependent cleanup; modify deleteRelease to accept a workspaceID and scope the DELETE (e.g. join resource.workspace_id) to prevent deleting releases across workspaces.
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/db/releases.go around lines 231 to 238, the
deleteRelease function currently deletes by release id only which can remove
releases across workspaces; change the function signature to accept a
workspaceID (e.g. deleteRelease(ctx context.Context, releaseID, workspaceID
string, tx pgx.Tx) error), update the DELETE SQL to scope by workspace (perform
a DELETE ... USING resource JOIN or subquery to ensure the release is tied to a
resource with resource.workspace_id = $2), and call tx.Exec with both parameters
(releaseID and workspaceID); update all call sites to pass workspaceID
accordingly.
Summary by CodeRabbit