refactor: move approval records to db#814
Merged
adityachoudhari26 merged 3 commits intomainfrom Feb 25, 2026
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a new UserApprovalRecord model, DB schema and migration, sqlc queries and generated code, DB-backed and in-memory repository implementations with wiring into the store and migration of legacy in-memory records; tests updated to use UUID-based user IDs. Changes
Sequence Diagram(s)sequenceDiagram
participant Restore as Store.Restore
participant InMem as InMemoryRepo
participant DBRepo as DBRepo
participant SQL as Database
participant Client as Client
rect rgba(200,200,255,0.5)
Restore->>InMem: Items() (legacy approvals)
InMem-->>Restore: map[string]*UserApprovalRecord
end
rect rgba(200,255,200,0.5)
Restore->>DBRepo: For each item: Set(record)
DBRepo->>SQL: UpsertUserApprovalRecord(...)
SQL-->>DBRepo: OK
DBRepo-->>Restore: success / log on error
end
rect rgba(255,200,200,0.5)
Client->>DBRepo: GetApprovedByVersionAndEnvironment(version,env)
DBRepo->>SQL: ListApprovedRecordsByVersionAndEnvironment(...)
SQL-->>DBRepo: rows
DBRepo-->>Client: []*UserApprovalRecord (converted to OAPI)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
packages/db/drizzle/0149_milky_hammerhead.sql (1)
1-9: Consider adding an index and foreign key constraints.
- The
GetApprovedByVersionAndEnvironmentquery filters on(version_id, environment_id, status). The composite PK(version_id, user_id, environment_id)won't efficiently serve this pattern sinceuser_idsits between the two needed columns. Consider adding an index:CREATE INDEX idx_user_approval_record_version_env ON user_approval_record (version_id, environment_id);
- There are no foreign key constraints referencing the parent tables for
version_id,user_id, orenvironment_id. If referential integrity is enforced at the application level this may be intentional, but worth confirming.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/drizzle/0149_milky_hammerhead.sql` around lines 1 - 9, Add a covering index to optimize the GetApprovedByVersionAndEnvironment pattern and add FK constraints for referential integrity: create an index named idx_user_approval_record_version_env on user_approval_record (version_id, environment_id, status) to support queries filtering by version_id+environment_id and status, and add FOREIGN KEY constraints on user_approval_record.version_id, user_id, and environment_id referencing the appropriate parent tables (use your project's canonical version, user, and environment primary key columns) while keeping the existing PRIMARY KEY constraint user_approval_record_version_id_user_id_environment_id_pk.apps/workspace-engine/pkg/db/queries/user_approval_records.sql (1)
16-20: Add an index for this filter+sort path.Line 16–20 filters by
(version_id, environment_id, status)and orders bycreated_at. Consider adding a composite index like(version_id, environment_id, status, created_at)in the migration to avoid scan/sort pressure as this table grows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/db/queries/user_approval_records.sql` around lines 16 - 20, Add a composite index to support the ListApprovedRecordsByVersionAndEnvironment query: create a migration that adds an index on user_approval_record covering (version_id, environment_id, status, created_at) (e.g., idx_user_approval_record_version_env_status_created_at) so the WHERE on version_id/environment_id/status and ORDER BY created_at can use an index scan and avoid sorting; include a down/migration rollback that drops the same index.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/workspace-engine/pkg/db/queries/schema.sql`:
- Around line 226-234: The primary key on user_approval_record is (version_id,
user_id, environment_id) which doesn't efficiently support reads by (version_id,
environment_id[, status]); add a dedicated lookup index on user_approval_record
to cover those queries (at minimum on (version_id, environment_id) and
preferably including status for filtered lookups) so reads by
version+environment (and optional status) are index-backed; create the index
using a non-blocking approach (e.g., CONCURRENTLY) and name it clearly (e.g.,
idx_user_approval_record_by_version_env[_status]) to locate it later.
In
`@apps/workspace-engine/pkg/workspace/store/repository/db/userapprovalrecords/mapper.go`:
- Around line 48-54: The code currently swallows parse errors for e.CreatedAt
leaving createdAt Invalid and causing a DB NOT NULL violation later; update the
mapper (the block that sets createdAt using time.Parse and pgtype.Timestamptz)
to check the parse error and return it (or wrap it with context) instead of
ignoring it—mirror the UUID parsing behavior above so that if
time.Parse(e.CreatedAt, time.RFC3339) fails the function returns an error with a
clear message referencing e.CreatedAt and the failing value rather than
proceeding with an invalid pgtype.Timestamptz.
In
`@apps/workspace-engine/pkg/workspace/store/repository/db/userapprovalrecords/repo.go`:
- Around line 33-35: In Get (repo.go) don’t swallow all DB errors as “not
found”: detect sql.ErrNoRows specifically (errors.Is(err, sql.ErrNoRows)) and
return the current nil,false for that case, but for any other error either
(preferred) change Get's signature to return the error and propagate it upward,
or at minimum log the unexpected error with context (include the record key/ID
and the error) using the repository logger (e.g., r.logger or similar) before
returning nil,false; update callers if you change the signature.
In `@apps/workspace-engine/pkg/workspace/store/user_approval_records.go`:
- Around line 37-39: UserApprovalRecords.Get currently constructs an invalid DB
key by concatenating versionId and userId only; update the method to include
environmentId and build a three-part, delimiter-separated key matching the DB
repo parser (e.g., environmentId + delimiter + versionId + delimiter + userId)
before calling u.repo.Get; change the Get signature (and all callers) to accept
environmentId or otherwise obtain it from the struct, and ensure the same
delimiter/ordering is used as the repo's Get parser so keys resolve correctly.
In `@apps/workspace-engine/test/integration/dbtest.go`:
- Line 82: The test now wires in store.WithDBUserApprovalRecords(ctx) which
populates the user_approval_record table but the teardown never deletes those
rows; add an explicit deletion of user_approval_record in the test cleanup block
(before deleting deployment_version and environment) so FK constraints don't
fail — locate the cleanup near the end of the test in dbtest.go and add a
deletion for user_approval_record (matching the other explicit table deletions),
and apply the same change to the other cleanup block referenced (lines ~103-119)
to keep both cleanups consistent.
---
Nitpick comments:
In `@apps/workspace-engine/pkg/db/queries/user_approval_records.sql`:
- Around line 16-20: Add a composite index to support the
ListApprovedRecordsByVersionAndEnvironment query: create a migration that adds
an index on user_approval_record covering (version_id, environment_id, status,
created_at) (e.g., idx_user_approval_record_version_env_status_created_at) so
the WHERE on version_id/environment_id/status and ORDER BY created_at can use an
index scan and avoid sorting; include a down/migration rollback that drops the
same index.
In `@packages/db/drizzle/0149_milky_hammerhead.sql`:
- Around line 1-9: Add a covering index to optimize the
GetApprovedByVersionAndEnvironment pattern and add FK constraints for
referential integrity: create an index named
idx_user_approval_record_version_env on user_approval_record (version_id,
environment_id, status) to support queries filtering by
version_id+environment_id and status, and add FOREIGN KEY constraints on
user_approval_record.version_id, user_id, and environment_id referencing the
appropriate parent tables (use your project's canonical version, user, and
environment primary key columns) while keeping the existing PRIMARY KEY
constraint user_approval_record_version_id_user_id_environment_id_pk.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (29)
apps/workspace-engine/pkg/db/models.goapps/workspace-engine/pkg/db/queries/schema.sqlapps/workspace-engine/pkg/db/queries/user_approval_records.sqlapps/workspace-engine/pkg/db/sqlc.yamlapps/workspace-engine/pkg/db/user_approval_records.sql.goapps/workspace-engine/pkg/persistence/integration_test.goapps/workspace-engine/pkg/workspace/store/repository/db/repo.goapps/workspace-engine/pkg/workspace/store/repository/db/userapprovalrecords/mapper.goapps/workspace-engine/pkg/workspace/store/repository/db/userapprovalrecords/repo.goapps/workspace-engine/pkg/workspace/store/repository/interfaces.goapps/workspace-engine/pkg/workspace/store/repository/memory/repo.goapps/workspace-engine/pkg/workspace/store/repository/repo.goapps/workspace-engine/pkg/workspace/store/store.goapps/workspace-engine/pkg/workspace/store/user_approval_records.goapps/workspace-engine/svc/workspaceconsumer/consumer.goapps/workspace-engine/test/e2e/engine_approval_records_test.goapps/workspace-engine/test/e2e/engine_policy_approval_test.goapps/workspace-engine/test/e2e/engine_policy_conflicts_test.goapps/workspace-engine/test/e2e/engine_policy_dynamic_changes_test.goapps/workspace-engine/test/e2e/engine_policy_skip_test.goapps/workspace-engine/test/e2e/engine_policy_version_cooldown_test.goapps/workspace-engine/test/e2e/engine_policy_versionselector_test.goapps/workspace-engine/test/e2e/engine_trace_test.goapps/workspace-engine/test/integration/dbtest.gopackages/db/drizzle/0149_milky_hammerhead.sqlpackages/db/drizzle/meta/0149_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/index.tspackages/db/src/schema/user-approval-record.ts
Comment on lines
+226
to
234
| CREATE TABLE user_approval_record ( | ||
| version_id UUID NOT NULL, | ||
| user_id UUID NOT NULL, | ||
| environment_id UUID NOT NULL, | ||
| status TEXT NOT NULL, | ||
| reason TEXT, | ||
| created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), | ||
| PRIMARY KEY (version_id, user_id, environment_id) | ||
| ); No newline at end of file |
Contributor
There was a problem hiding this comment.
Add a lookup index for approver queries.
Line 233 defines PK order (version_id, user_id, environment_id), which does not efficiently support reads by (version_id, environment_id[, status]). This will degrade as approval rows grow.
💡 Proposed schema addition
CREATE TABLE user_approval_record (
version_id UUID NOT NULL,
user_id UUID NOT NULL,
environment_id UUID NOT NULL,
status TEXT NOT NULL,
reason TEXT,
created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(),
PRIMARY KEY (version_id, user_id, environment_id)
);
+
+CREATE INDEX user_approval_record_version_env_status_idx
+ ON user_approval_record (version_id, environment_id, status);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| CREATE TABLE user_approval_record ( | |
| version_id UUID NOT NULL, | |
| user_id UUID NOT NULL, | |
| environment_id UUID NOT NULL, | |
| status TEXT NOT NULL, | |
| reason TEXT, | |
| created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), | |
| PRIMARY KEY (version_id, user_id, environment_id) | |
| ); | |
| CREATE TABLE user_approval_record ( | |
| version_id UUID NOT NULL, | |
| user_id UUID NOT NULL, | |
| environment_id UUID NOT NULL, | |
| status TEXT NOT NULL, | |
| reason TEXT, | |
| created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), | |
| PRIMARY KEY (version_id, user_id, environment_id) | |
| ); | |
| CREATE INDEX user_approval_record_version_env_status_idx | |
| ON user_approval_record (version_id, environment_id, status); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/db/queries/schema.sql` around lines 226 - 234, The
primary key on user_approval_record is (version_id, user_id, environment_id)
which doesn't efficiently support reads by (version_id, environment_id[,
status]); add a dedicated lookup index on user_approval_record to cover those
queries (at minimum on (version_id, environment_id) and preferably including
status for filtered lookups) so reads by version+environment (and optional
status) are index-backed; create the index using a non-blocking approach (e.g.,
CONCURRENTLY) and name it clearly (e.g.,
idx_user_approval_record_by_version_env[_status]) to locate it later.
apps/workspace-engine/pkg/workspace/store/repository/db/userapprovalrecords/mapper.go
Show resolved
Hide resolved
apps/workspace-engine/pkg/workspace/store/repository/db/userapprovalrecords/repo.go
Show resolved
Hide resolved
Comment on lines
37
to
39
| func (u *UserApprovalRecords) Get(versionId, userId string) (*oapi.UserApprovalRecord, bool) { | ||
| return u.repo.UserApprovalRecords.Get(versionId + userId) | ||
| return u.repo.Get(versionId + userId) | ||
| } |
Contributor
There was a problem hiding this comment.
Get is building an invalid key for the DB-backed implementation.
At Line 38, versionId + userId omits environmentId and has no structured delimiter/formatter. The DB repo’s Get parses a 3-part key, so this path will fail to resolve records correctly.
Fix direction
-func (u *UserApprovalRecords) Get(versionId, userId string) (*oapi.UserApprovalRecord, bool) {
- return u.repo.Get(versionId + userId)
+func (u *UserApprovalRecords) Get(versionId, userId, environmentId string) (*oapi.UserApprovalRecord, bool) {
+ return u.repo.Get(buildUserApprovalRecordKey(versionId, userId, environmentId))
}
+
+func buildUserApprovalRecordKey(versionID, userID, environmentID string) string {
+ // Must exactly match the parser format used by db/userapprovalrecords.parseKey.
+ return versionID + ":" + userID + ":" + environmentID
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/workspace/store/user_approval_records.go` around
lines 37 - 39, UserApprovalRecords.Get currently constructs an invalid DB key by
concatenating versionId and userId only; update the method to include
environmentId and build a three-part, delimiter-separated key matching the DB
repo parser (e.g., environmentId + delimiter + versionId + delimiter + userId)
before calling u.repo.Get; change the Get signature (and all callers) to accept
environmentId or otherwise obtain it from the struct, and ensure the same
delimiter/ordering is used as the repo's Get parser so keys resolve correctly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/workspace-engine/pkg/workspace/store/user_approval_records.go (1)
54-81:⚠️ Potential issue | 🟡 MinorNil dereference risk in
GetApproversandGetApprovalRecords.
GetApprovedByVersionAndEnvironmentreturns[]*oapi.UserApprovalRecord. Both methods access fields on those pointers without nil guards:r.UserId(line 62) andrecords[i].CreatedAt/records[j].CreatedAt(lines 74–75) will panic if any element is nil.🛠️ Proposed fix
approvers := make([]string, len(records)) for i, r := range records { + if r == nil { + continue + } approvers[i] = r.UserId }sort.Slice(records, func(i, j int) bool { + if records[i] == nil || records[j] == nil { + return records[i] == nil + } ti, ei := time.Parse(time.RFC3339, records[i].CreatedAt)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/user_approval_records.go` around lines 54 - 81, The repo call GetApprovedByVersionAndEnvironment returns []*oapi.UserApprovalRecord which may contain nil elements; update GetApprovers and GetApprovalRecords to first filter out nil records (e.g., build a new slice recordsFiltered from records where r != nil) before using r.UserId or accessing records[i].CreatedAt/records[j].CreatedAt, then use that filtered slice for building approvers and for sort.Slice; in the sort comparator also defensively handle any remaining nils by ordering nils consistently (or falling back to comparing CreatedAt strings) and keep the existing time.Parse fallback logic. Ensure returned slices reflect the filtered data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/workspace-engine/pkg/workspace/store/user_approval_records.go`:
- Around line 37-38: The Get method builds a composite key with "-" delimiters
which breaks parseKey (expects 108-char concatenated UUIDs); update
UserApprovalRecords.Get to construct the key by concatenating versionId, userId,
and environmentId with no separators (versionId+userId+environmentId) so
parseKey receives the expected 108-char string—note Remove already passes keys
in that format and ToUpsertParams handles UUIDs separately.
---
Outside diff comments:
In `@apps/workspace-engine/pkg/workspace/store/user_approval_records.go`:
- Around line 54-81: The repo call GetApprovedByVersionAndEnvironment returns
[]*oapi.UserApprovalRecord which may contain nil elements; update GetApprovers
and GetApprovalRecords to first filter out nil records (e.g., build a new slice
recordsFiltered from records where r != nil) before using r.UserId or accessing
records[i].CreatedAt/records[j].CreatedAt, then use that filtered slice for
building approvers and for sort.Slice; in the sort comparator also defensively
handle any remaining nils by ordering nils consistently (or falling back to
comparing CreatedAt strings) and keep the existing time.Parse fallback logic.
Ensure returned slices reflect the filtered data.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/workspace-engine/pkg/workspace/store/repository/db/userapprovalrecords/mapper.goapps/workspace-engine/pkg/workspace/store/repository/db/userapprovalrecords/repo.goapps/workspace-engine/pkg/workspace/store/user_approval_records.goapps/workspace-engine/test/integration/dbtest.go
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/workspace-engine/pkg/workspace/store/repository/db/userapprovalrecords/repo.go
- apps/workspace-engine/pkg/workspace/store/repository/db/userapprovalrecords/mapper.go
apps/workspace-engine/pkg/workspace/store/user_approval_records.go
Outdated
Show resolved
Hide resolved
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 by CodeRabbit