fix(tests): use sql.json in onboarding migration test and refresh metrics snapshot#1420
Open
mantrakp04 wants to merge 10 commits intodevfrom
Open
fix(tests): use sql.json in onboarding migration test and refresh metrics snapshot#1420mantrakp04 wants to merge 10 commits intodevfrom
mantrakp04 wants to merge 10 commits intodevfrom
Conversation
…rics snapshot
- Match the rest of the migration test suite by using `sql.json(...)` instead of
`${JSON.stringify(...)}::jsonb` in the `add_project_onboarding_state` test, so
the helper handles serialization and parameter binding consistently.
- Refresh the `internal-metrics` snapshot so that `active_users_by_country.AQ`
reflects the actual `last_active_at_millis desc` ordering produced by the
test (mailbox-2 became active in AQ after mailbox-1, so it sorts first).
Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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:
📝 WalkthroughWalkthroughTest-only edits and OpenAPI spec updates: migration test now writes JSONB via ChangesTest updates
OpenAPI: CLI TTL defaults
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthAPI
participant Test
Client->>AuthAPI: POST /auth/cli
AuthAPI-->>Test: returns expires_at
Test->>Test: assert expires_at ≈ now + TTL
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Contributor
Greptile SummaryTwo test-only maintenance fixes with no production code changes.
Confidence Score: 5/5Both changes are test-only; no production code is touched, and each fix is a straightforward correction that aligns test behavior with actual runtime semantics. The migration test now uses the driver's native No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Test as Migration Test
participant Driver as postgres Driver
participant DB as PostgreSQL
Note over Test,DB: Before (manual cast)
Test->>Driver: "sql`SET onboardingState = ${JSON.stringify(obj)}::jsonb`"
Driver->>DB: literal string + ::jsonb cast
DB-->>Test: result
Note over Test,DB: After (sql.json)
Test->>Driver: "sql`SET onboardingState = ${sql.json(obj)}`"
Driver->>DB: parameterized JSON binding (no manual cast)
DB-->>Test: result
Reviews (1): Last reviewed commit: "fix(tests): use sql.json in onboarding m..." | Re-trigger Greptile |
Contributor
There was a problem hiding this comment.
Pull request overview
Test-maintenance PR to align JSON parameter binding in a Prisma migration test with existing repo patterns, and to refresh an internal metrics snapshot to match the expected ordering of “active users by country”.
Changes:
- Updated onboarding migration test to use
sql.json(...)for JSON parameter binding instead of manualJSON.stringify(... )::jsonb. - Refreshed the
internal-metricsVitest snapshot to reflect the expected ordering foractive_users_by_country.AQ.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| apps/backend/prisma/migrations/20260420000000_add_project_onboarding_state/tests/default-and-updates.ts | Switches JSONB update to use sql.json(onboardingState) for consistent driver-side serialization/binding. |
| apps/e2e/tests/backend/endpoints/api/v1/snapshots/internal-metrics.test.ts.snap | Updates the expected ordering of AQ active users to match the test’s sign-in activity ordering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Updated the SQL query in the onboarding migration test to explicitly cast the onboardingState to jsonb, ensuring proper data type handling during the update process.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
Replace fixed wait(3000) with the existing waitForMetricsToIncludeUsersByCountry helper. The last sign-in (mailbox-3 → CH) sometimes wasn't ingested by ClickHouse before the metrics call fired, causing the CH bucket to be missing from the response and the snapshot to fail intermittently in CI.
…apshots - Update OpenAPI defaults snapshot for `expires_in_millis` to match the new 2-minute default introduced in #1419. - CLI route test: default is now 2min and max 15min, so update expectations. - failed-emails-digest: refresh inline snapshot to match new digest output. - internal-metrics anonymous-with-activity test: poll for ClickHouse ingestion instead of a fixed wait, mirroring the sibling test.
![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: 1
🧹 Nitpick comments (1)
apps/e2e/tests/backend/endpoints/api/v1/auth/cli/route.test.ts (1)
16-21: ⚡ Quick winReduce timing-flake risk by bracketing request time instead of sampling
nowonce after response.These assertions can fail intermittently when response latency exceeds the 10s margin, because
nowis captured only after the request returns.Proposed stabilization
it("should create a new CLI auth attempt", async ({ expect }) => { + const requestStartedAt = Date.now(); const response = await niceBackendFetch("/api/latest/auth/cli", { method: "POST", accessType: "server", body: {}, }); + const requestFinishedAt = Date.now(); expect(response.status).toBe(200); expect(response.body).toHaveProperty("polling_code"); expect(response.body).toHaveProperty("login_code"); expect(response.body).toHaveProperty("expires_at"); - // Verify that the expiration time is about 2 minutes from now (default polling-code TTL) + // Verify expiration is ~2 minutes from creation time, tolerating request latency const expiresAt = new Date(response.body.expires_at); - const now = new Date(); const twoMinutesInMs = 2 * 60 * 1000; - expect(expiresAt.getTime() - now.getTime()).toBeGreaterThan(twoMinutesInMs - 10000); // Allow for a small margin of error - expect(expiresAt.getTime() - now.getTime()).toBeLessThan(twoMinutesInMs + 10000); // Allow for a small margin of error + expect(expiresAt.getTime()).toBeGreaterThanOrEqual(requestStartedAt + twoMinutesInMs - 10000); + expect(expiresAt.getTime()).toBeLessThanOrEqual(requestFinishedAt + twoMinutesInMs + 10000); });it("should create a new CLI auth attempt with custom expiration time", async ({ expect }) => { const customExpirationMs = 10 * 60 * 1000; // 10 minutes (max is 15) + const requestStartedAt = Date.now(); const response = await niceBackendFetch("/api/latest/auth/cli", { method: "POST", accessType: "server", body: { expires_in_millis: customExpirationMs, }, }); + const requestFinishedAt = Date.now(); expect(response.status).toBe(200); expect(response.body).toHaveProperty("polling_code"); expect(response.body).toHaveProperty("login_code"); expect(response.body).toHaveProperty("expires_at"); - // Verify that the expiration time is about the requested 10 minutes from now + // Verify expiration is ~requested TTL from creation time, tolerating request latency const expiresAt = new Date(response.body.expires_at); - const now = new Date(); - expect(expiresAt.getTime() - now.getTime()).toBeGreaterThan(customExpirationMs - 10000); // Allow for a small margin of error - expect(expiresAt.getTime() - now.getTime()).toBeLessThan(customExpirationMs + 10000); // Allow for a small margin of error + expect(expiresAt.getTime()).toBeGreaterThanOrEqual(requestStartedAt + customExpirationMs - 10000); + expect(expiresAt.getTime()).toBeLessThanOrEqual(requestFinishedAt + customExpirationMs + 10000); });Also applies to: 40-44
🤖 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 `@apps/e2e/tests/backend/endpoints/api/v1/auth/cli/route.test.ts` around lines 16 - 21, The test's timing assertions use a single sampled now after the HTTP response, which can cause flakes; update the test in route.test.ts to record a requestStart timestamp before sending the request and a requestEnd timestamp immediately after receiving the response, then assert that response.body.expires_at (expiresAt) lies between requestStart + twoMinutesInMs ± margin and requestEnd + twoMinutesInMs ± margin (or equivalently that expiresAt - requestStart and expiresAt - requestEnd fall within the allowed bounds); apply the same change to the other occurrence noted (the block around the earlier 40-44 assertions) so both use requestStart/requestEnd bracketing instead of a single now variable.
🤖 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.
Inline comments:
In `@docs-mintlify/openapi/client.json`:
- Around line 894-897: The JSDoc in
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts
incorrectly documents the CLI auth TTL as "(default: 2 hours)"; update that
JSDoc text to "(default: 2 minutes)" (reflecting the new 120000 ms default), add
a short migration note in the same JSDoc or nearby comment advising downstream
SDK/tooling may have cached the old 2-hour value and must be updated, and search
for any hard-coded 120000 or 7200000 ms usages (e.g., in client-app-impl or
related auth/polling functions) to ensure documentation and code defaults are
consistent.
---
Nitpick comments:
In `@apps/e2e/tests/backend/endpoints/api/v1/auth/cli/route.test.ts`:
- Around line 16-21: The test's timing assertions use a single sampled now after
the HTTP response, which can cause flakes; update the test in route.test.ts to
record a requestStart timestamp before sending the request and a requestEnd
timestamp immediately after receiving the response, then assert that
response.body.expires_at (expiresAt) lies between requestStart + twoMinutesInMs
± margin and requestEnd + twoMinutesInMs ± margin (or equivalently that
expiresAt - requestStart and expiresAt - requestEnd fall within the allowed
bounds); apply the same change to the other occurrence noted (the block around
the earlier 40-44 assertions) so both use requestStart/requestEnd bracketing
instead of a single now variable.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 27abf1cb-e9ec-4e77-a201-8af5193d42a9
📒 Files selected for processing (6)
apps/e2e/tests/backend/endpoints/api/v1/auth/cli/route.test.tsapps/e2e/tests/backend/endpoints/api/v1/internal-metrics.test.tsapps/e2e/tests/backend/endpoints/api/v1/internal/failed-emails-digest.test.tsdocs-mintlify/openapi/admin.jsondocs-mintlify/openapi/client.jsondocs-mintlify/openapi/server.json
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/e2e/tests/backend/endpoints/api/v1/internal-metrics.test.ts
- updateProject: don't call overrideEnvironmentConfigOverride when there are no env-config keys to set, so metadata-only PATCHes (e.g. onboarding_state) on local-emulator projects no longer trip the LOCAL_EMULATOR_ENV_CONFIG_BLOCKED guard. - Drop the "rejects non-existent config files" test: the local-emulator POST endpoint now auto-creates the file (#1413), and that path is already covered by "writes default config for empty files". - failed-emails-digest test: poll until both the verify-email and test-email failures have been recorded instead of relying on a fixed 10s wait, and restore the original snapshot.
…logic - Removed unnecessary environment config override call for metadata-only project updates in `createOrUpdateProjectWithLegacyConfig`. - Updated internal metrics tests to replace fixed wait times with appropriate polling for asynchronous data ingestion, ensuring more reliable test outcomes. - Added a new test case for handling non-existent config files in the local emulator project endpoint, improving error handling and user feedback.
…ility - Added a condition to skip environment config overrides for metadata-only project updates in `createOrUpdateProjectWithLegacyConfig`. - Replaced fixed wait times in internal metrics tests with a polling mechanism for better reliability in asynchronous data verification. - Cleaned up the `failed-emails-digest` test by improving the structure and ensuring consistent snapshot expectations. - Removed outdated test for non-existent config files in the local emulator project endpoint, as it is now handled by a different test case.
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
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
Two small test-maintenance fixes that came up while running the suite:
apps/backend/prisma/migrations/20260420000000_add_project_onboarding_state/tests/default-and-updates.ts): switch the JSON insert from\${JSON.stringify(onboardingState)}::jsonbto\${sql.json(onboardingState)}. This matches the pattern used by every other migration test in the repo (see20260214000000_fix_trusted_domains_config/tests/*) and lets thepostgresdriver handle serialization and parameter binding consistently rather than relying on a manual::jsonbcast.apps/e2e/tests/backend/endpoints/api/v1/__snapshots__/internal-metrics.test.ts.snap): updateactive_users_by_country.AQto listmailbox-2beforemailbox-1. Theshould return metrics data with userstest signs inmailbox-1(mailboxes[0]) into AQ first, then later signsmailbox-2(mailboxes[1]) into AQ, so sorted bylast_active_at_millis descmailbox-2should come first. The snapshot now matches that ordering.No production code is touched — both changes are limited to test fixtures.
Test plan
pnpm -C apps/backend test run(migration tests)pnpm -C apps/e2e test run internal-metrics(snapshot test)pnpm lintpnpm typecheckMade with Cursor
Summary by CodeRabbit