Draft
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an onboarding feature: new protobuf messages and RPCs (Get/Create/Finish), generated Connect client/server artifacts, a DB migration and Drizzle schema for an Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
comatory
force-pushed
the
ondrej/eng-9192-onboarding-wizard-from-signup-to-live-metrics-01
branch
from
9b1427c to
1094f7a
Compare
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (18.06%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2719 +/- ##
===========================================
- Coverage 63.46% 40.20% -23.26%
===========================================
Files 251 985 +734
Lines 26767 123868 +97101
Branches 0 5571 +5571
===========================================
+ Hits 16987 49803 +32816
- Misses 8416 72378 +63962
- Partials 1364 1687 +323
🚀 New features to boost your workflow:
|
![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: 3
🧹 Nitpick comments (2)
proto/wg/cosmo/platform/v1/platform.proto (1)
3197-3202: Missing field number 2 inGetOnboardingResponse.Field numbers skip from 1 to 3, leaving field 2 unused. For a new message definition, this is unusual and could indicate an accidental omission or a removed field. If a field was intentionally removed during development, consider adding a
reserved 2;statement to document this and prevent accidental reuse. If this was unintentional, renumber the fields sequentially.Option 1: Add reserved statement if field was intentionally removed
message GetOnboardingResponse { Response response = 1; + reserved 2; optional string finishedAt = 3; int32 federatedGraphsCount = 4; bool enabled = 5; }Option 2: Renumber fields sequentially if this is a new message
message GetOnboardingResponse { Response response = 1; - optional string finishedAt = 3; - int32 federatedGraphsCount = 4; - bool enabled = 5; + optional string finishedAt = 2; + int32 federatedGraphsCount = 3; + bool enabled = 4; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proto/wg/cosmo/platform/v1/platform.proto` around lines 3197 - 3202, GetOnboardingResponse currently skips field number 2 (fields go 1, 3, 4, 5); either explicitly reserve the missing tag or renumber fields—if the field was intentionally removed, add a `reserved 2;` declaration inside the GetOnboardingResponse message to prevent reuse, otherwise renumber fields so they are sequential (e.g., change finishedAt from tag 3 to tag 2 and adjust any dependent code or clients that reference these tags).controlplane/src/core/bufservices/onboarding/createOnboarding.ts (1)
36-36: Track the TODO with a concrete follow-up item.Line 36 documents missing invitation/rename handling in a user-facing onboarding path; please capture this as a tracked issue before rollout.
If you want, I can draft the issue description and acceptance criteria.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/bufservices/onboarding/createOnboarding.ts` at line 36, Replace the ambiguous TODO in createOnboarding.ts with a tracked issue reference: create a ticket in your issue tracker describing the missing "invitation flow + organization renaming" work, include acceptance criteria (invite acceptance path, org rename UX, API behavior, edge cases, tests), then update the TODO comment in the createOnboarding function/file to reference the created ticket ID (e.g., TODO(issue-123): handle invitation flow + organization renaming) so the follow-up is discoverable and actionable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controlplane/src/core/repositories/OnboardingRepository.ts`:
- Around line 12-18: The insert into schema.onboarding is passing a non-existent
column "step" which causes insert/type errors; in the call to
this.db.insert(schema.onboarding).values(...) remove the step property from the
payload (and any related typing or interface that includes step for this
operation) so the object only contains valid columns (userId, organizationId,
slack, email); update any tests or callers expecting step to be stored if
necessary.
- Around line 11-19: The create() method can raise a DB unique-constraint error
on duplicate (user_id, organization_id, version); make it idempotent by changing
the insertion logic to perform an upsert / conflict-ignore on schema.onboarding
(or first check for an existing onboarding row for this.organizationId and
userId/version and return success if found) instead of blindly calling
this.db.insert; update the code around create(), this.db.insert and
schema.onboarding to use a "do nothing on conflict" or equivalent upsert
behavior so retries/double-submits simply return success rather than error.
In `@proto/wg/cosmo/platform/v1/platform.proto`:
- Around line 3204-3209: The CreateOnboardingRequest message contains a typo:
rename the field invititationEmails to invitationEmails (keep the same tag
number 2) so generated code uses the correct name; update all references/usages
of CreateOnboardingRequest.invititationEmails to
CreateOnboardingRequest.invitationEmails and regenerate protobuf artifacts and
client code to propagate the change.
---
Nitpick comments:
In `@controlplane/src/core/bufservices/onboarding/createOnboarding.ts`:
- Line 36: Replace the ambiguous TODO in createOnboarding.ts with a tracked
issue reference: create a ticket in your issue tracker describing the missing
"invitation flow + organization renaming" work, include acceptance criteria
(invite acceptance path, org rename UX, API behavior, edge cases, tests), then
update the TODO comment in the createOnboarding function/file to reference the
created ticket ID (e.g., TODO(issue-123): handle invitation flow + organization
renaming) so the follow-up is discoverable and actionable.
In `@proto/wg/cosmo/platform/v1/platform.proto`:
- Around line 3197-3202: GetOnboardingResponse currently skips field number 2
(fields go 1, 3, 4, 5); either explicitly reserve the missing tag or renumber
fields—if the field was intentionally removed, add a `reserved 2;` declaration
inside the GetOnboardingResponse message to prevent reuse, otherwise renumber
fields so they are sequential (e.g., change finishedAt from tag 3 to tag 2 and
adjust any dependent code or clients that reference these tags).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a5f51bdd-8da4-461d-9a11-640537da36fb
⛔ Files ignored due to path filters (2)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**connect-go/gen/proto/wg/cosmo/platform/v1/platformv1connect/platform.connect.gois excluded by!**/gen/**
📒 Files selected for processing (12)
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.tsconnect/src/wg/cosmo/platform/v1/platform_connect.tsconnect/src/wg/cosmo/platform/v1/platform_pb.tscontrolplane/migrations/0137_icy_sasquatch.sqlcontrolplane/migrations/meta/0137_snapshot.jsoncontrolplane/migrations/meta/_journal.jsoncontrolplane/src/core/bufservices/PlatformService.tscontrolplane/src/core/bufservices/onboarding/createOnboarding.tscontrolplane/src/core/bufservices/onboarding/getOnboarding.tscontrolplane/src/core/repositories/OnboardingRepository.tscontrolplane/src/db/schema.tsproto/wg/cosmo/platform/v1/platform.proto
Comment on lines
+3204
to
+3209
| message CreateOnboardingRequest { | ||
| string organizationName = 1; | ||
| repeated string invititationEmails = 2; | ||
| bool slack = 3; | ||
| bool email = 4; | ||
| } |
Contributor
There was a problem hiding this comment.
Typo in field name: invititationEmails should be invitationEmails.
The field name has an extra 't' - invititationEmails instead of invitationEmails. This typo will propagate to generated code and API clients, making it awkward to use and potentially confusing.
Proposed fix
message CreateOnboardingRequest {
string organizationName = 1;
- repeated string invititationEmails = 2;
+ repeated string invitationEmails = 2;
bool slack = 3;
bool email = 4;
}📝 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
| message CreateOnboardingRequest { | |
| string organizationName = 1; | |
| repeated string invititationEmails = 2; | |
| bool slack = 3; | |
| bool email = 4; | |
| } | |
| message CreateOnboardingRequest { | |
| string organizationName = 1; | |
| repeated string invitationEmails = 2; | |
| bool slack = 3; | |
| bool email = 4; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@proto/wg/cosmo/platform/v1/platform.proto` around lines 3204 - 3209, The
CreateOnboardingRequest message contains a typo: rename the field
invititationEmails to invitationEmails (keep the same tag number 2) so generated
code uses the correct name; update all references/usages of
CreateOnboardingRequest.invititationEmails to
CreateOnboardingRequest.invitationEmails and regenerate protobuf artifacts and
client code to propagate the change.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
proto/wg/cosmo/platform/v1/platform.proto (1)
3204-3207:⚠️ Potential issue | 🟡 MinorFix typo in onboarding request field name before codegen.
invititationEmailscontains a spelling error and will generate awkward API fields across clients. Rename it toinvitationEmailswhile keeping tag2.Proposed fix
message CreateOnboardingRequest { string organizationName = 1; - repeated string invititationEmails = 2; + repeated string invitationEmails = 2; bool slack = 3; bool email = 4; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proto/wg/cosmo/platform/v1/platform.proto` around lines 3204 - 3207, The CreateOnboardingRequest proto has a misspelled field name invititationEmails; rename this field to invitationEmails in the message definition (message CreateOnboardingRequest) but preserve the field number 2 so generated APIs keep wire compatibility; update any references to CreateOnboardingRequest.invititationEmails in the repo (tests, docs, usages) to the new CreateOnboardingRequest.invitationEmails before running codegen.
🧹 Nitpick comments (1)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (1)
25042-25047: Addreserved 2;declaration or correct field numbering inGetOnboardingResponse.Field number 2 is skipped (numbering jumps from 1 to 3), which is inconsistent with the sequential field numbering used throughout the rest of the proto file. If this is intentional backward compatibility, add
reserved 2;to document it. If unintentional, correct the numbering before the API is released.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connect/src/wg/cosmo/platform/v1/platform_pb.ts` around lines 25042 - 25047, The generated proto message for GetOnboardingResponse (the static readonly fields array in its TypeScript class) skips field number 2 — update the proto mapping to either declare `reserved 2;` in the GetOnboardingResponse proto to document the intentional gap or renumber the existing fields so they are sequential (change the field currently using number 3/finishedAt to 2 if that was unintentional); locate the static readonly fields definition for GetOnboardingResponse (the FieldList initializer shown with entries for "response", "finishedAt", "federatedGraphsCount", "enabled") and apply the appropriate change so the TypeScript fields reflect the corrected proto schema.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@connect/src/wg/cosmo/platform/v1/platform_pb.ts`:
- Around line 25075-25078: The generated TypeScript field invititationEmails
contains a typo and must be renamed to invitationEmails in the source .proto and
regenerated; open the proto message that defines the field name (currently
`invititationEmails`), correct the identifier to `invitationEmails`, re-run the
protobuf/TS generation to update the generated symbol in platform_pb.ts (the
field and any accessor names), then update all code references to use
invitationEmails (or add a temporary compatibility alias/mapping from the old
name while you update callers) so the public API uses the correct name.
In `@controlplane/src/core/bufservices/onboarding/createOnboarding.ts`:
- Around line 36-43: The onboarding handler is dropping advertised request
fields (organizationName and invitation emails) when calling
OnboardingRepository.create; update the create call to pass req.organizationName
and req.invitationEmails (or req.invitations) alongside userId/slack/email, and
then update OnboardingRepository.create (and its persisted model/schema) to
accept and persist organizationName and invitations (or return a validation
error if you choose to reject them). Ensure the symbols updated are the
CreateOnboardingRequest usage in createOnboarding.ts, the onboardingRepo.create
invocation, and the OnboardingRepository.create method/DTO/schema so the fields
are carried end-to-end.
- Around line 38-43: The onboarding insertion can throw uniqueness errors on
repeated requests; update the flow so duplicate-create is mapped to a
deterministic domain response instead of bubbling as a generic error: either
implement/use an idempotent repository method (e.g.,
OnboardingRepository.findOrCreate or upsert) that returns the existing row when
one exists, or wrap the call to onboardingRepo.create in a try/catch and detect
the DB unique-constraint error (translate it into a specific domain result like
"AlreadyOnboarded" or return the existing onboarding record), ensuring you
reference onboardingRepo.create and authContext.userId so callers receive a
stable idempotent success rather than a raw DB error.
---
Duplicate comments:
In `@proto/wg/cosmo/platform/v1/platform.proto`:
- Around line 3204-3207: The CreateOnboardingRequest proto has a misspelled
field name invititationEmails; rename this field to invitationEmails in the
message definition (message CreateOnboardingRequest) but preserve the field
number 2 so generated APIs keep wire compatibility; update any references to
CreateOnboardingRequest.invititationEmails in the repo (tests, docs, usages) to
the new CreateOnboardingRequest.invitationEmails before running codegen.
---
Nitpick comments:
In `@connect/src/wg/cosmo/platform/v1/platform_pb.ts`:
- Around line 25042-25047: The generated proto message for GetOnboardingResponse
(the static readonly fields array in its TypeScript class) skips field number 2
— update the proto mapping to either declare `reserved 2;` in the
GetOnboardingResponse proto to document the intentional gap or renumber the
existing fields so they are sequential (change the field currently using number
3/finishedAt to 2 if that was unintentional); locate the static readonly fields
definition for GetOnboardingResponse (the FieldList initializer shown with
entries for "response", "finishedAt", "federatedGraphsCount", "enabled") and
apply the appropriate change so the TypeScript fields reflect the corrected
proto schema.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e187db28-20bf-4d40-b2fd-3d4ccda93887
⛔ Files ignored due to path filters (2)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**connect-go/gen/proto/wg/cosmo/platform/v1/platformv1connect/platform.connect.gois excluded by!**/gen/**
📒 Files selected for processing (8)
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.tsconnect/src/wg/cosmo/platform/v1/platform_connect.tsconnect/src/wg/cosmo/platform/v1/platform_pb.tscontrolplane/src/core/bufservices/PlatformService.tscontrolplane/src/core/bufservices/onboarding/createOnboarding.tscontrolplane/src/core/bufservices/onboarding/getOnboarding.tscontrolplane/src/core/repositories/OnboardingRepository.tsproto/wg/cosmo/platform/v1/platform.proto
🚧 Files skipped from review as they are similar to previous changes (4)
- controlplane/src/core/bufservices/PlatformService.ts
- controlplane/src/core/repositories/OnboardingRepository.ts
- connect/src/wg/cosmo/platform/v1/platform_connect.ts
- controlplane/src/core/bufservices/onboarding/getOnboarding.ts
Comment on lines
+25075
to
+25078
| /** | ||
| * @generated from field: repeated string invititationEmails = 2; | ||
| */ | ||
| invititationEmails: string[] = []; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find the proto file containing the typo and verify its location
# Search for the typo in proto files
rg -n "invititationEmails" --type-add 'proto:*.proto' --type proto
# Also search in case it's defined elsewhere
rg -n "invititationEmails" -g '*.proto'Repository: wundergraph/cosmo
Length of output: 238
Fix typo in field name: invititationEmails should be invitationEmails.
The field has an extra 'i' in the source .proto file at proto/wg/cosmo/platform/v1/platform.proto:3206. This typo will become part of the public API contract and should be corrected before release.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@connect/src/wg/cosmo/platform/v1/platform_pb.ts` around lines 25075 - 25078,
The generated TypeScript field invititationEmails contains a typo and must be
renamed to invitationEmails in the source .proto and regenerated; open the proto
message that defines the field name (currently `invititationEmails`), correct
the identifier to `invitationEmails`, re-run the protobuf/TS generation to
update the generated symbol in platform_pb.ts (the field and any accessor
names), then update all code references to use invitationEmails (or add a
temporary compatibility alias/mapping from the old name while you update
callers) so the public API uses the correct name.
Comment on lines
+36
to
+43
| // TODO: handle invitation flow + organization renaming | ||
|
|
||
| const onboardingRepo = new OnboardingRepository(opts.db, authContext.organizationId); | ||
| await onboardingRepo.create({ | ||
| userId: authContext.userId, | ||
| slack: req.slack, | ||
| email: req.email, | ||
| }); |
Contributor
There was a problem hiding this comment.
CreateOnboarding drops request data that the API contract advertises.
organizationName and invitation emails are accepted in CreateOnboardingRequest but ignored here, and the repository persists only userId, slack, and email. This creates a misleading RPC contract and silent data loss from caller input.
I can draft a follow-up patch (or issue) to either persist these fields end-to-end or remove them from the RPC until implemented.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controlplane/src/core/bufservices/onboarding/createOnboarding.ts` around
lines 36 - 43, The onboarding handler is dropping advertised request fields
(organizationName and invitation emails) when calling
OnboardingRepository.create; update the create call to pass req.organizationName
and req.invitationEmails (or req.invitations) alongside userId/slack/email, and
then update OnboardingRepository.create (and its persisted model/schema) to
accept and persist organizationName and invitations (or return a validation
error if you choose to reject them). Ensure the symbols updated are the
CreateOnboardingRequest usage in createOnboarding.ts, the onboardingRepo.create
invocation, and the OnboardingRepository.create method/DTO/schema so the fields
are carried end-to-end.
controlplane/src/core/bufservices/onboarding/createOnboarding.ts
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
comatory
force-pushed
the
ondrej/eng-9192-onboarding-wizard-from-signup-to-live-metrics-01
branch
from
ec7ea95 to
b6fb36f
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
controlplane/src/core/repositories/OnboardingRepository.ts (2)
19-29: Add contextual try/catch around DB operations.Repository methods currently bubble raw DB exceptions directly. Wrapping with contextual errors here improves diagnosability and keeps failure handling consistent.
As per coding guidelines: "Add proper error handling with try-catch blocks".
Also applies to: 32-39, 42-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/repositories/OnboardingRepository.ts` around lines 19 - 29, Wrap the DB upsert call in OnboardingRepository (the await this.db.insert(schema.onboarding).values(values).onConflictDoUpdate(...) sequence) in a try/catch so raw DB exceptions are captured and re-thrown or wrapped with contextual information; in the catch, include identifying details (userId, organizationId, version) and the original error when constructing the new error or logging, then rethrow the contextual error so callers get consistent, diagnosable failures; apply the same pattern to the other repository methods noted (the DB calls around lines 32-39 and 42-46).
11-11: Use an interface for input shape and explicit return types on methods.
createOrUpdateuses an inline object type, and all public methods rely on inferred returns. Extracting an interface and adding explicitPromise<...>return types will align this file with TS repo standards.♻️ Proposed typing cleanup
+interface CreateOrUpdateInput { + userId: string; + slack: boolean; + email: boolean; +} + +type OnboardingRow = typeof schema.onboarding.$inferSelect; + export class OnboardingRepository { @@ - public async createOrUpdate({ userId, slack, email }: { userId: string; slack: boolean; email: boolean }) { + public async createOrUpdate({ userId, slack, email }: CreateOrUpdateInput): Promise<void> { @@ - public async finish(userId: string) { + public async finish(userId: string): Promise<OnboardingRow | undefined> { @@ - public async getByUserId(userId: string) { + public async getByUserId(userId: string): Promise<OnboardingRow | undefined> {As per coding guidelines: "Prefer interfaces over type aliases for object shapes in TypeScript" and "Use explicit type annotations for function parameters and return types in TypeScript".
Also applies to: 31-31, 41-41
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/repositories/OnboardingRepository.ts` at line 11, Extract the inline parameter shape into a named interface (e.g. OnboardingCreateOrUpdateInput with userId: string, slack: boolean, email: boolean) and update the createOrUpdate method signature to accept that interface (createOrUpdate(input: OnboardingCreateOrUpdateInput)): add an explicit Promise return type (e.g. createOrUpdate(...): Promise<Onboarding> or the concrete domain type this repository returns). Do the same explicit Promise<...> return-type annotation for the other public methods in this class (the public methods declared at the other two public method locations referenced in the review), replacing inferred returns with concrete Promise types matching their actual return values. Ensure the new interface and explicit return types are exported/visible as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controlplane/src/core/repositories/OnboardingRepository.ts`:
- Around line 31-36: The schema includes a version column in the unique
constraint but methods finish() and getByUserId() never use or increment it,
causing ambiguity; update either the data operations to be explicit about
versioning (filter by version in getByUserId() and finish(), and increment
version when creating/updating rows via the repository functions such as
finish() and any create/update methods) or simplify the schema by removing the
version column and its unique constraint so the repository only relies on
(userId, organizationId); locate references to schema.onboarding, finish(), and
getByUserId() and apply the chosen approach consistently across all repository
methods.
---
Nitpick comments:
In `@controlplane/src/core/repositories/OnboardingRepository.ts`:
- Around line 19-29: Wrap the DB upsert call in OnboardingRepository (the await
this.db.insert(schema.onboarding).values(values).onConflictDoUpdate(...)
sequence) in a try/catch so raw DB exceptions are captured and re-thrown or
wrapped with contextual information; in the catch, include identifying details
(userId, organizationId, version) and the original error when constructing the
new error or logging, then rethrow the contextual error so callers get
consistent, diagnosable failures; apply the same pattern to the other repository
methods noted (the DB calls around lines 32-39 and 42-46).
- Line 11: Extract the inline parameter shape into a named interface (e.g.
OnboardingCreateOrUpdateInput with userId: string, slack: boolean, email:
boolean) and update the createOrUpdate method signature to accept that interface
(createOrUpdate(input: OnboardingCreateOrUpdateInput)): add an explicit Promise
return type (e.g. createOrUpdate(...): Promise<Onboarding> or the concrete
domain type this repository returns). Do the same explicit Promise<...>
return-type annotation for the other public methods in this class (the public
methods declared at the other two public method locations referenced in the
review), replacing inferred returns with concrete Promise types matching their
actual return values. Ensure the new interface and explicit return types are
exported/visible as needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1317b5bb-eba4-46be-925f-dd7217887250
⛔ Files ignored due to path filters (2)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**connect-go/gen/proto/wg/cosmo/platform/v1/platformv1connect/platform.connect.gois excluded by!**/gen/**
📒 Files selected for processing (9)
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.tsconnect/src/wg/cosmo/platform/v1/platform_connect.tsconnect/src/wg/cosmo/platform/v1/platform_pb.tscontrolplane/src/core/bufservices/PlatformService.tscontrolplane/src/core/bufservices/onboarding/createOnboarding.tscontrolplane/src/core/bufservices/onboarding/finishOnboarding.tscontrolplane/src/core/bufservices/onboarding/getOnboarding.tscontrolplane/src/core/repositories/OnboardingRepository.tsproto/wg/cosmo/platform/v1/platform.proto
✅ Files skipped from review due to trivial changes (1)
- controlplane/src/core/bufservices/onboarding/createOnboarding.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- controlplane/src/core/bufservices/onboarding/getOnboarding.ts
- controlplane/src/core/bufservices/PlatformService.ts
- controlplane/src/core/bufservices/onboarding/finishOnboarding.ts
- proto/wg/cosmo/platform/v1/platform.proto
- connect/src/wg/cosmo/platform/v1/platform_connect.ts
Comment on lines
+31
to
+36
| public async finish(userId: string) { | ||
| const result = await this.db | ||
| .update(schema.onboarding) | ||
| .set({ finishedAt: new Date() }) | ||
| .where(and(eq(schema.onboarding.organizationId, this.organizationId), eq(schema.onboarding.userId, userId))) | ||
| .returning(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether onboarding supports multiple versions and whether callers pass version context.
# 1) Inspect onboarding schema/migration for `version` defaults/constraints.
fd -i 'schema.ts' controlplane/src/db --exec sed -n '1,260p' {}
fd -i '*.sql' controlplane/migrations --exec rg -n -C3 'onboarding|version|unique' {}
# 2) Inspect repository and handler call sites for version-aware APIs.
rg -n -C3 'class OnboardingRepository|createOrUpdate\(|finish\(|getByUserId\(' controlplane/src/core/repositories controlplane/src/core/bufservices
# Expected:
# - If multiple versions are possible, repository methods should include a version predicate.
# - If only a single hardcoded version is allowed, enforce/encode that invariant explicitly.Repository: wundergraph/cosmo
Length of output: 14297
🏁 Script executed:
# Find the onboarding table definition in schema
rg -n 'export const onboarding' controlplane/src/db/schema.ts -A 30Repository: wundergraph/cosmo
Length of output: 1051
🏁 Script executed:
# Check if there are any version-related columns or constraints in onboarding
rg -n 'version' controlplane/src/core/repositories/OnboardingRepository.ts -B 5 -A 5Repository: wundergraph/cosmo
Length of output: 370
🏁 Script executed:
# Look at the complete OnboardingRepository implementation
cat -n controlplane/src/core/repositories/OnboardingRepository.tsRepository: wundergraph/cosmo
Length of output: 1770
🏁 Script executed:
# Check the createOnboarding service to see what initial version is set
cat -n controlplane/src/core/bufservices/onboarding/createOnboarding.ts | head -80Repository: wundergraph/cosmo
Length of output: 2226
Clarify the intent of the version column: it's part of the unique constraint but never incremented or filtered.
The version column exists in the schema with default(1) and is part of the unique constraint (userId, organizationId, version), yet your code never increments it and the finish() and getByUserId() methods don't filter by version. While this is not a functional bug today (only version=1 rows are created), the design is confusing and fragile. If version-based operations are intended, add explicit version filtering to these methods. Otherwise, remove the version column to clarify the schema.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controlplane/src/core/repositories/OnboardingRepository.ts` around lines 31 -
36, The schema includes a version column in the unique constraint but methods
finish() and getByUserId() never use or increment it, causing ambiguity; update
either the data operations to be explicit about versioning (filter by version in
getByUserId() and finish(), and increment version when creating/updating rows
via the repository functions such as finish() and any create/update methods) or
simplify the schema by removing the version column and its unique constraint so
the repository only relies on (userId, organizationId); locate references to
schema.onboarding, finish(), and getByUserId() and apply the chosen approach
consistently across all repository methods.
b6fb36f to
54189f0
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
controlplane/src/core/repositories/OnboardingRepository.ts (1)
42-48: Minor: The?? undefinedis redundant.
findFirstalready returnsundefinedwhen no match is found, so the nullish coalescing is unnecessary.♻️ Simplify the return
public async getByUserId(userId: string) { - const result = await this.db.query.onboarding.findFirst({ + return this.db.query.onboarding.findFirst({ where: and(eq(schema.onboarding.organizationId, this.organizationId), eq(schema.onboarding.userId, userId)), }); - - return result ?? undefined; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/repositories/OnboardingRepository.ts` around lines 42 - 48, The getByUserId method unnecessarily uses "return result ?? undefined" after this.db.query.onboarding.findFirst which already yields undefined on no match; simplify by returning the result directly from getByUserId (update the method with "return result;" in the getByUserId function to remove the redundant nullish coalescing).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controlplane/src/core/repositories/OnboardingRepository.ts`:
- Around line 22-29: The onConflictDoUpdate in OnboardingRepository's
createOrUpdate currently resets finishedAt to null on every conflict which can
revert completed onboarding; update the conflict-handling so finishedAt is
preserved by removing finishedAt from the set map in onConflictDoUpdate (i.e.,
stop writing finishedAt there) or change the update to conditionally set
finishedAt = CASE WHEN schema.onboarding.finishedAt IS NULL THEN NULL ELSE
schema.onboarding.finishedAt END (or equivalent ORM conditional) so existing
non-null finishedAt is kept; alternatively, add a guard in the calling handler
to avoid calling OnboardingRepository.createOrUpdate for already-finished
onboarding records.
---
Nitpick comments:
In `@controlplane/src/core/repositories/OnboardingRepository.ts`:
- Around line 42-48: The getByUserId method unnecessarily uses "return result ??
undefined" after this.db.query.onboarding.findFirst which already yields
undefined on no match; simplify by returning the result directly from
getByUserId (update the method with "return result;" in the getByUserId function
to remove the redundant nullish coalescing).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 678bbb39-83b6-4034-9114-a6765cacb9e5
⛔ Files ignored due to path filters (2)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**connect-go/gen/proto/wg/cosmo/platform/v1/platformv1connect/platform.connect.gois excluded by!**/gen/**
📒 Files selected for processing (9)
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.tsconnect/src/wg/cosmo/platform/v1/platform_connect.tsconnect/src/wg/cosmo/platform/v1/platform_pb.tscontrolplane/src/core/bufservices/PlatformService.tscontrolplane/src/core/bufservices/onboarding/createOnboarding.tscontrolplane/src/core/bufservices/onboarding/finishOnboarding.tscontrolplane/src/core/bufservices/onboarding/getOnboarding.tscontrolplane/src/core/repositories/OnboardingRepository.tsproto/wg/cosmo/platform/v1/platform.proto
✅ Files skipped from review due to trivial changes (2)
- controlplane/src/core/bufservices/PlatformService.ts
- connect/src/wg/cosmo/platform/v1/platform_connect.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- controlplane/src/core/bufservices/onboarding/finishOnboarding.ts
- controlplane/src/core/bufservices/onboarding/createOnboarding.ts
Comment on lines
+22
to
+29
| .onConflictDoUpdate({ | ||
| target: [schema.onboarding.userId, schema.onboarding.organizationId, schema.onboarding.version], | ||
| set: { | ||
| slack, | ||
| email, | ||
| finishedAt: null, | ||
| }, | ||
| }); |
Contributor
There was a problem hiding this comment.
Resetting finishedAt to null on conflict may unintentionally revert completed onboarding.
When createOrUpdate is called after a user has already finished onboarding, the onConflictDoUpdate will reset finishedAt back to null, effectively marking the onboarding as incomplete. This could lead to the user seeing the onboarding wizard again unexpectedly.
Consider either:
- Preserving
finishedAtwhen it already has a value (don't include it in thesetclause) - Adding a guard in the handler to prevent calling
createOrUpdateon already-finished onboarding - Using a conditional update that only sets
finishedAt: nullif it was previouslynull
🛡️ Option 1: Remove finishedAt from conflict update
.onConflictDoUpdate({
target: [schema.onboarding.userId, schema.onboarding.organizationId, schema.onboarding.version],
set: {
slack,
email,
- finishedAt: null,
},
});📝 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
| .onConflictDoUpdate({ | |
| target: [schema.onboarding.userId, schema.onboarding.organizationId, schema.onboarding.version], | |
| set: { | |
| slack, | |
| email, | |
| finishedAt: null, | |
| }, | |
| }); | |
| .onConflictDoUpdate({ | |
| target: [schema.onboarding.userId, schema.onboarding.organizationId, schema.onboarding.version], | |
| set: { | |
| slack, | |
| email, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controlplane/src/core/repositories/OnboardingRepository.ts` around lines 22 -
29, The onConflictDoUpdate in OnboardingRepository's createOrUpdate currently
resets finishedAt to null on every conflict which can revert completed
onboarding; update the conflict-handling so finishedAt is preserved by removing
finishedAt from the set map in onConflictDoUpdate (i.e., stop writing finishedAt
there) or change the update to conditionally set finishedAt = CASE WHEN
schema.onboarding.finishedAt IS NULL THEN NULL ELSE schema.onboarding.finishedAt
END (or equivalent ORM conditional) so existing non-null finishedAt is kept;
alternatively, add a guard in the calling handler to avoid calling
OnboardingRepository.createOrUpdate for already-finished onboarding records.
54189f0 to
f039f02
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
comatory
force-pushed
the
ondrej/eng-9192-onboarding-wizard-from-signup-to-live-metrics-01
branch
from
4d962cc to
ae0d1a7
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
comatory
force-pushed
the
ondrej/eng-9192-onboarding-wizard-from-signup-to-live-metrics-01
branch
from
7e83557 to
cb36c42
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
comatory
force-pushed
the
ondrej/eng-9192-onboarding-wizard-from-signup-to-live-metrics-01
branch
11 times, most recently
from
4b81c23 to
d6e81f0
Compare
comatory
force-pushed
the
ondrej/eng-9192-onboarding-wizard-from-signup-to-live-metrics-01
branch
from
d6e81f0 to
30179c4
Compare
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
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.
PR (1) for onboarding wizard: database changes + RPC
Stack: