[codex] Make onboarding demo reset provenance-based#2300
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR replaces onboarding demo reset with provenance tracking. A new migration introduces an ChangesSafe Onboarding Demo Data Reset
Sequence Diagram(s)sequenceDiagram
participant Client
participant ResetFunc as reset_onboarding_demo_app_data
participant LegacyClaim as claim_legacy_onboarding_demo_data
participant ProvenanceTable as onboarding_demo_data
participant DataTables as demo data tables
Client->>ResetFunc: call with app_uuid
ResetFunc->>LegacyClaim: rebuild legacy provenance
LegacyClaim->>ProvenanceTable: upsert legacy row markers
ResetFunc->>DataTables: check guards (cascade safety)
DataTables-->>ResetFunc: assert no untracked cascade
ResetFunc->>DataTables: delete tracked demo rows
ResetFunc->>ProvenanceTable: remove tracked rows
ResetFunc-->>Client: reset complete
sequenceDiagram
participant createDemoApp as createDemoApp()
participant seedTxn as seedOnboardingDemoDataInTransaction()
participant pgClient as Postgres Client
participant tracking as track_onboarding_demo_data
createDemoApp->>createDemoApp: generate demoSeedId + prepare version rows
createDemoApp->>createDemoApp: prepare manifest/channel/deploy rows
createDemoApp->>createDemoApp: prepare device/daily chart rows
createDemoApp->>createDemoApp: prepare build request rows
createDemoApp->>seedTxn: pass all row arrays + demoSeedId
seedTxn->>pgClient: BEGIN transaction
seedTxn->>pgClient: bulk insert all data via jsonb_to_recordset
seedTxn->>tracking: call track_onboarding_demo_data for each relation
seedTxn->>pgClient: clear app_metrics_cache for org
seedTxn->>pgClient: COMMIT or ROLLBACK
seedTxn-->>createDemoApp: return insert counts
createDemoApp-->>createDemoApp: log transaction committed summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/onboarding-demo-reset.test.ts (1)
63-64: ⚡ Quick winConsider using
it.concurrent()for parallel test execution.Per coding guidelines, tests should use
it.concurrent()instead ofit()to run tests in parallel within the same file for faster CI/CD. Each test creates isolated data with unique app IDs, so they can safely run concurrently.-describe('onboarding demo reset', () => { - it('deletes tracked demo rows while preserving real app data', async () => { +describe('onboarding demo reset', () => { + it.concurrent('deletes tracked demo rows while preserving real app data', async () => {Apply the same change to all other
it()calls in this file (lines 325, 399, 459, 539, 635, 727, 775).As per coding guidelines: "use it.concurrent() instead of it() to run tests in parallel within the same file for faster CI/CD"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/onboarding-demo-reset.test.ts` around lines 63 - 64, Replace all synchronous test declarations using it(...) with concurrent test declarations it.concurrent(...) in this file so the tests run in parallel; specifically update the top-level test starting with the "onboarding demo reset" describe and every other it() call referenced (the tests beginning around lines 63, 325, 399, 459, 539, 635, 727, 775) by changing the call site to use it.concurrent and keeping the same async test bodies and names (no other logic changes required).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/onboarding-demo-reset.test.ts`:
- Around line 63-64: Replace all synchronous test declarations using it(...)
with concurrent test declarations it.concurrent(...) in this file so the tests
run in parallel; specifically update the top-level test starting with the
"onboarding demo reset" describe and every other it() call referenced (the tests
beginning around lines 63, 325, 399, 459, 539, 635, 727, 775) by changing the
call site to use it.concurrent and keeping the same async test bodies and names
(no other logic changes required).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2dce202-4597-4dee-acc8-fbc563053081
📒 Files selected for processing (8)
cli/README.mdcli/package.jsoncli/webdocs/build.mdxpackage.jsonsupabase/functions/_backend/public/app/demo.tssupabase/functions/_backend/utils/version.tssupabase/migrations/20260519123613_safe_demo_data_reset.sqltests/onboarding-demo-reset.test.ts
💤 Files with no reviewable changes (2)
- cli/README.md
- cli/webdocs/build.mdx
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/onboarding-demo-reset.test.ts (1)
583-610:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the legacy build markers unique and match migration patterns.
These two
it.concurrent()tests still insert the samebuilder_job_idandupload_session_keyliterals. Reusing fixed seed identifiers breaks the parallel-safe test-data rule and can become flaky ifbuild_requestsconstrains either field or matches on them outsideapp_id. The migration's demo data tracking uses strictLIKEpatterns ('demo-job-%'and'demo-session-%'), so unique values must preserve these prefixes.Suggested change
- 'demo-job-legacy', - 'demo-session-legacy', + `demo-job-onboarding-demo-reset-${randomUUID()}`, + `demo-session-onboarding-demo-reset-${randomUUID()}`,Also applies to: 675-702
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/onboarding-demo-reset.test.ts` around lines 583 - 610, The test inserts reuse fixed literals for builder_job_id and upload_session_key ("demo-job-legacy" and "demo-session-legacy") which breaks parallel-safe test-data; change the values generated in the executeSQL call so they remain prefixed with "demo-job-" and "demo-session-" but are unique per test run (e.g. append appId, a timestamp or random suffix). Update the INSERT call used by the it.concurrent() tests (the executeSQL invocation that sets builder_job_id and upload_session_key) to build unique strings while preserving the required LIKE prefixes so the migration patterns ('demo-job-%' and 'demo-session-%') still match.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/onboarding-demo-reset.test.ts`:
- Around line 583-610: The test inserts reuse fixed literals for builder_job_id
and upload_session_key ("demo-job-legacy" and "demo-session-legacy") which
breaks parallel-safe test-data; change the values generated in the executeSQL
call so they remain prefixed with "demo-job-" and "demo-session-" but are unique
per test run (e.g. append appId, a timestamp or random suffix). Update the
INSERT call used by the it.concurrent() tests (the executeSQL invocation that
sets builder_job_id and upload_session_key) to build unique strings while
preserving the required LIKE prefixes so the migration patterns ('demo-job-%'
and 'demo-session-%') still match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26c272fa-64ec-450b-aa75-3238c87cd6e8
📒 Files selected for processing (2)
supabase/functions/_backend/public/app/demo.tstests/onboarding-demo-reset.test.ts
💤 Files with no reviewable changes (1)
- supabase/functions/_backend/public/app/demo.ts
|
1 participant
Summary (AI generated)
Motivation (AI generated)
The previous reset path used app-wide cleanup keyed by app_id and need_onboarding. That is unsafe once a pending onboarding app can contain both demo and production data. Resetting demo data needs row provenance, and deleting parent demo rows must not cascade into untracked user-owned child rows.
Business Impact (AI generated)
This protects customer app versions, channels, devices, deploy history, build rows, and analytics from being removed during onboarding/demo resets. It reduces support and restoration risk after demo onboarding, while keeping demo data resettable for sales/onboarding flows.
Test Plan (AI generated)
bun lintbun lint:backendbunx eslint tests/onboarding-demo-reset.test.tsbun typecheckbun run supabase:db:resetbun run supabase:with-env -- bunx vitest run tests/onboarding-demo-reset.test.tsbun run supabase:with-env -- bunx vitest run tests/build_time_tracking.test.ts tests/cron_stat_org.test.ts tests/private-error-cases.test.tsbun test:backendGenerated with AI
Summary by CodeRabbit
New Features
Bug Fixes
Tests