refactor(stats): migrate version tracking to use version_name#1459
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughMigrates analytics from numeric Changes
Sequence Diagram(s)mermaid Client->>Plugin: POST install/uninstall with versionOnly (version_name) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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. 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f60dcaaa7c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // eslint-disable-next-line ts/no-explicit-any | ||
| (supabase.from('daily_version') as any) | ||
| .upsert(resolvedVersionUsage, { onConflict: 'app_id,date,version_name' }) | ||
| .eq('app_id', body.appId) |
There was a problem hiding this comment.
The new resolvedVersionUsage objects only carry version_name, but daily_version still has version_id defined as NOT NULL and in the primary key (see base migration). This upsert will therefore insert NULL version_id values and fail, which means cron_stat_app will stop persisting daily version stats. Either relax/remove the version_id constraint in the migration or include a populated version_id when writing.
Useful? React with 👍 / 👎.
| // Handle backwards compatibility: old Cloudflare data has numeric version_id in blob2, | ||
| // new data has version_name string. Detect and resolve old data. | ||
| const versionNamesToResolve = versionUsage | ||
| .filter(v => /^\d+$/.test(String(v.version_name))) | ||
| .map(v => Number(v.version_name)) |
There was a problem hiding this comment.
The legacy-detection logic assumes any numeric version_name is an old version_id. Apps that use numeric-only version names (e.g., "1", "2") will be misclassified and remapped via app_versions IDs, which can silently shift their stats to the wrong version during the transition period. Consider using an explicit marker for old data or a safer discriminator than /^\d+$/.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR migrates version statistics tracking from using version_id (numeric database primary key) to version_name (string) to eliminate unnecessary database reads during stats collection. The change affects both Postgres and Cloudflare Analytics Engine data storage and includes backwards compatibility logic to handle the transition period.
Changes:
- Database migration adds
version_namecolumns toversion_usageanddaily_versiontables with backfill logic and updated SQL functions - Backend stats collection now uses
version_namefrom request bodies instead of looking upversion_idfrom database - Cloudflare Analytics Engine writes now store version names as strings instead of numeric IDs
- Cron jobs include backwards compatibility logic to detect and resolve old numeric version IDs to version names during aggregation
- Frontend queries updated to use
version_nameinstead ofversion_idfor statistics retrieval
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| supabase/migrations/20260118005052_version_usage_use_version_name.sql | Adds version_name columns, backfills existing data, updates SQL function, creates indexes and partial unique constraint |
| supabase/functions/_backend/utils/update.ts | Changes createStatsVersion call to pass version.name instead of version.id |
| supabase/functions/_backend/utils/types.ts | Adds VersionUsage interface with version_name field |
| supabase/functions/_backend/utils/supabase.ts | Updates trackVersionUsageSB to accept version_name parameter and use type casting for stale types |
| supabase/functions/_backend/utils/stats.ts | Updates createStatsVersion signature to accept version_name instead of version_id |
| supabase/functions/_backend/utils/cloudflare.ts | Updates trackVersionUsageCF to store version_name in Analytics Engine, removes internal VersionUsageCF interface |
| supabase/functions/_backend/triggers/cron_stat_app.ts | Adds backwards compatibility logic to detect numeric version IDs and resolve them to version names before aggregation |
| supabase/functions/_backend/triggers/cron_email.ts | Updates filter logic to match by version_name or numeric version_id for backwards compatibility |
| supabase/functions/_backend/public/statistics/index.ts | Refactors bundle usage calculation to use version_name directly, eliminating version lookup queries |
| supabase/functions/_backend/plugins/stats.ts | Changes stats calls to use versionOnly (string from request) instead of appVersion.id |
| src/services/supabase.ts | Updates frontend interface and query to use version_name instead of version_id |
| // Filter by version_name (new format) OR version_id as string (old Cloudflare format) | ||
| // This handles backwards compatibility during the transition period | ||
| const versionIdStr = versionId ? String(versionId) : null | ||
| const installs = versionStats | ||
| .filter(row => Number(row.version_id) === Number(versionId)) | ||
| .filter(row => row.version_name === versionName || (versionIdStr && row.version_name === versionIdStr)) | ||
| .reduce((sum, row) => sum + (row.install ?? 0), 0) |
There was a problem hiding this comment.
The backwards compatibility filter allows matching by either version_name or versionIdStr (the old numeric format). However, the logic uses OR condition which means if both match different records, they would both be included in the aggregation. During the transition period, this could lead to double-counting if the same version exists in both formats with the same data. Consider using exclusive OR logic or adding a comment explaining why this scenario won't occur.
| // Query uses version_name column - cast needed because auto-generated types are stale | ||
| const { data: rawDailyVersion, error: dailyVersionError } = await supabase | ||
| .from('daily_version') | ||
| .select('date, app_id, version_id, install, uninstall') | ||
| .select('date, app_id, version_name, install, uninstall') | ||
| .eq('app_id', appId) | ||
| .gte('date', from.toISOString()) | ||
| .lte('date', to.toISOString()) | ||
| .not('version_name', 'is', null) |
There was a problem hiding this comment.
The query explicitly filters out NULL version_names with .not('version_name', 'is', null), but the migration allows version_name to be nullable for backwards compatibility. This filter means old data (with NULL version_name) won't be returned. While this is intentional per the migration design, it creates a discrepancy during the transition period where old statistics won't be visible in the frontend. Consider if this is the desired behavior or if the query should use the same COALESCE pattern as the SQL function to show both old and new data.
| export async function getDailyVersion(appId: string, startDate?: string, endDate?: string): Promise<AppUsageByVersion[]> { | ||
| // Query uses version_name column - cast needed because auto-generated types are stale | ||
| const { data, error } = await useSupabase() | ||
| .from('daily_version') | ||
| .select('date, app_id, version_id, install, uninstall') | ||
| .select('date, app_id, version_name, install, uninstall') | ||
| .eq('app_id', appId) | ||
| .gte('date', startDate) | ||
| .lte('date', endDate) | ||
| .not('version_name', 'is', null) | ||
| .order('date', { ascending: true }) | ||
|
|
||
| if (error || !data) { | ||
| console.error('Error fetching data from daily_version:', error) | ||
| return [] | ||
| } | ||
| return data | ||
| // Cast to our interface - the SQL table has version_name but auto-generated types are stale | ||
| return data as unknown as AppUsageByVersion[] |
There was a problem hiding this comment.
The same issue exists here - filtering with .not('version_name', 'is', null) excludes old data with NULL version_name. This creates inconsistency with the backend's SQL function (read_version_usage) which handles both old and new data via COALESCE. The frontend will miss historical statistics during the transition period. Either update this query to use a similar COALESCE approach via RPC call, or ensure this is the intended behavior and document it.
| // eslint-disable-next-line ts/no-explicit-any | ||
| (supabase.from('daily_version') as any) | ||
| .upsert(resolvedVersionUsage, { onConflict: 'app_id,date,version_name' }) |
There was a problem hiding this comment.
Using eslint-disable-next-line ts/no-explicit-any with a type cast to any bypasses TypeScript's type safety completely. While this is necessary due to stale auto-generated types, consider using a more specific interface or type assertion that at least validates the structure of the data being passed. This would catch potential issues if the upsert payload structure changes.
| SET version_name = av.name | ||
| FROM "public"."app_versions" av | ||
| WHERE dv.version_id = av.id AND dv.version_name IS NULL; | ||
|
|
There was a problem hiding this comment.
The backfill UPDATE statement uses a LEFT JOIN in the SQL function but the UPDATE itself only updates rows WHERE dv.version_id = av.id. This means if a version_id in daily_version doesn't have a matching record in app_versions (orphaned data), the backfill will silently skip it, leaving version_name as NULL. Consider adding error handling or logging to identify orphaned records, or change the UPDATE to set a placeholder value like 'unknown' for unmatched version_ids.
| -- 3b. For orphaned version_ids (no matching app_versions row), set a placeholder name | |
| UPDATE "public"."daily_version" dv | |
| SET version_name = 'unknown' | |
| WHERE dv.version_name IS NULL AND dv.version_id IS NOT NULL; |
| -- 6. Add unique constraint on (app_id, date, version_name) for upsert operations | ||
| -- First drop the old primary key constraint since we're changing the conflict key | ||
| -- Note: We keep version_id for backwards compatibility but add version_name as unique | ||
| CREATE UNIQUE INDEX IF NOT EXISTS "idx_daily_version_app_date_version_name_unique" ON "public"."daily_version" ("app_id", "date", "version_name") WHERE "version_name" IS NOT NULL; |
There was a problem hiding this comment.
The partial unique index only enforces uniqueness when version_name IS NOT NULL, but during the transition period there will be old records with NULL version_name and non-NULL version_id. These old records won't have uniqueness constraints, potentially allowing duplicate entries for the same (app_id, date, version_id) combination. Consider also adding a similar partial unique index for version_id to maintain data integrity during the transition period: CREATE UNIQUE INDEX IF NOT EXISTS "idx_daily_version_app_date_version_id_unique" ON "public"."daily_version" ("app_id", "date", "version_id") WHERE "version_id" IS NOT NULL AND "version_name" IS NULL;
| CREATE UNIQUE INDEX IF NOT EXISTS "idx_daily_version_app_date_version_name_unique" ON "public"."daily_version" ("app_id", "date", "version_name") WHERE "version_name" IS NOT NULL; | |
| CREATE UNIQUE INDEX IF NOT EXISTS "idx_daily_version_app_date_version_name_unique" ON "public"."daily_version" ("app_id", "date", "version_name") WHERE "version_name" IS NOT NULL; | |
| CREATE UNIQUE INDEX IF NOT EXISTS "idx_daily_version_app_date_version_id_unique" ON "public"."daily_version" ("app_id", "date", "version_id") WHERE "version_id" IS NOT NULL AND "version_name" IS NULL; |
| version_name: versionName, | ||
| app_id: appId, | ||
| action, | ||
| }, | ||
| } as unknown as { version_id: number, app_id: string, action: typeof action }, |
There was a problem hiding this comment.
Type casting with as unknown as is used here to bypass TypeScript type checking for stale auto-generated types. While the comment explains this, the casting signature doesn't match the actual table schema. The cast claims the object has version_id (number), but the actual inserted data has version_name (string). This mismatch could mask real type errors. Consider using a more accurate cast or defining a proper interface that matches the actual insert structure.
| // Handle backwards compatibility: old Cloudflare data has numeric version_id in blob2, | ||
| // new data has version_name string. Detect and resolve old data. | ||
| const versionNamesToResolve = versionUsage | ||
| .filter(v => /^\d+$/.test(String(v.version_name))) | ||
| .map(v => Number(v.version_name)) | ||
|
|
||
| let versionIdToNameMap: Record<number, string> = {} | ||
| if (versionNamesToResolve.length > 0) { | ||
| const { data: versions } = await supabase | ||
| .from('app_versions') | ||
| .select('id, name') | ||
| .in('id', versionNamesToResolve) | ||
| if (versions) { | ||
| versionIdToNameMap = Object.fromEntries(versions.map(v => [v.id, v.name])) | ||
| } | ||
| } |
There was a problem hiding this comment.
The backwards compatibility logic correctly detects numeric version IDs with /^\d+$/.test(), but there's a potential issue: if a legitimate version_name happens to be purely numeric (e.g., '123' as a version name), it will be incorrectly treated as an old version_id and trigger a database lookup that will fail. While unlikely, consider adding additional validation or documenting this constraint that version names should not be purely numeric strings.
| const versionStats = await readStatsVersion(c, appId, windowStart, windowEnd) | ||
| // Filter by version_name (new format) OR version_id as string (old Cloudflare format) | ||
| // This handles backwards compatibility during the transition period | ||
| const versionIdStr = versionId ? String(versionId) : null |
There was a problem hiding this comment.
This use of variable 'versionId' always evaluates to true.
riderx
force-pushed
the
riderx/stats-version-name
branch
from
910b260 to
115c894
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@supabase/functions/_backend/plugins/stats.ts`:
- Around line 126-134: Normalize old_version_name the same way
version_name/versionOnly is normalized before using it for uninstall stats:
strip any composite suffix (e.g., split on ':' and take the base version) and
use that normalized value when calling getAppVersionPostgres(old_version_name,
...) and createStatsVersion(..., old_version_name, 'uninstall') and when pushing
to statsActions; update the logic around the old_version_name check in the set
branch (where createStatsVersion, getAppVersionPostgres, appVersion, and
statsActions are used) so all references use the normalized_old_version_name.
♻️ Duplicate comments (7)
supabase/functions/_backend/utils/supabase.ts (1)
771-786: Type cast still mismatches the schema.The insert payload uses
version_namebut the cast claims{ version_id: number }, which can hide real type errors. Consider a temporary type that matches the new column.♻️ Safer interim typing
- { - version_name: versionName, - app_id: appId, - action, - } as unknown as { version_id: number, app_id: string, action: typeof action }, + { + version_name: versionName, + app_id: appId, + action, + } as unknown as { version_name: string, app_id: string, action: typeof action },src/services/supabase.ts (1)
269-286: Query migration looks good; confirm backfill coverage.The query correctly fetches
version_namewith the type cast to handle stale generated types. The.not('version_name', 'is', null)filter is intentional to return only properly migrated data.As noted in a previous review, records where the migration's backfill couldn't find a matching
app_versionsentry will haveNULLversion_nameand be silently excluded. Ensure the migration handles or logs orphanedversion_idrecords.supabase/functions/_backend/triggers/cron_stat_app.ts (2)
63-78: Numeric version name detection may misclassify legitimate version names.As flagged in a prior review, the regex
/^\d+$/.test()treats any purely numericversion_nameas a legacyversion_id. Apps that legitimately use numeric version names (e.g.,"1","123") will have their stats incorrectly remapped. Consider an alternative discriminator, such as checking whether the numeric value exceeds a reasonable threshold or using an explicit marker in the data.#!/bin/bash # Check if any apps use purely numeric version names that could be misclassified ast-grep --pattern 'app_versions' | head -20 # Search for version names in app_versions to see naming patterns rg -n "name" supabase/seed.sql 2>/dev/null | head -30 # Check the app_versions table schema for version naming constraints fd -t f -e sql . supabase/migrations | xargs rg -l "app_versions" | head -5 | xargs -I{} rg -A5 "CREATE TABLE.*app_versions" {}
132-136: Verifydaily_version.version_idconstraint allows NULL inserts.As flagged previously, the
resolvedVersionUsageobjects no longer includeversion_id. If thedaily_versiontable has aNOT NULLconstraint onversion_id(from the original schema), this upsert will fail at runtime. Confirm the migration relaxes theversion_idconstraint or that the original schema allowedNULL.#!/bin/bash # Check the original daily_version table definition for version_id constraint rg -n "daily_version" supabase/migrations --type sql | head -20 # Find the CREATE TABLE statement for daily_version fd -t f -e sql . supabase/migrations | xargs rg -B2 -A15 "CREATE TABLE.*daily_version" | head -40supabase/migrations/20260118005052_version_usage_use_version_name.sql (2)
10-14: Backfill leaves orphaned version_ids with NULL version_name.As noted previously, the
UPDATEonly setsversion_namewhere a matchingapp_versionsrecord exists. Orphanedversion_idvalues (e.g., deleted versions) will remain withNULLversion_nameand be excluded from queries that filter onversion_name IS NOT NULL.Consider adding a follow-up update to set a placeholder (e.g.,
'deleted_<version_id>') for orphaned records:UPDATE "public"."daily_version" dv SET version_name = 'deleted_' || dv.version_id::text WHERE dv.version_name IS NULL AND dv.version_id IS NOT NULL;
64-67: Partial unique index may leave old data without uniqueness enforcement.As flagged previously, the partial unique index only covers rows where
version_name IS NOT NULL. During the transition period, records withversion_idbutNULLversion_name(orphaned or un-migrated data) won't have a uniqueness constraint, potentially allowing duplicates on(app_id, date, version_id).If data integrity for legacy records is required, consider adding a complementary index:
CREATE UNIQUE INDEX IF NOT EXISTS "idx_daily_version_app_date_version_id_unique" ON "public"."daily_version" ("app_id", "date", "version_id") WHERE "version_id" IS NOT NULL AND "version_name" IS NULL;supabase/functions/_backend/public/statistics/index.ts (1)
325-338: Query correctly migrated to useversion_name.The
.not('version_name', 'is', null)filter is intentional to return only migrated data. As noted in a prior review, this means old data withNULLversion_name(e.g., orphaned records not backfilled) won't appear in the frontend during the transition period.The type cast at line 338 is necessary due to stale auto-generated types and is appropriately documented.
| if (action === 'set' && !device.is_emulator && device.is_prod) { | ||
| await createStatsVersion(c, appVersion.id, app_id, 'install') | ||
| // Use versionOnly (from request body) instead of appVersion - no DB read needed for stats | ||
| await createStatsVersion(c, versionOnly, app_id, 'install') | ||
| if (old_version_name) { | ||
| const oldVersion = await getAppVersionPostgres(c, app_id, old_version_name, undefined, drizzleClient as ReturnType<typeof getDrizzleClient>) | ||
| if (oldVersion && oldVersion.id !== appVersion.id) { | ||
| await createStatsVersion(c, oldVersion.id, app_id, 'uninstall') | ||
| await createStatsVersion(c, old_version_name, app_id, 'uninstall') | ||
| statsActions.push({ action: 'uninstall', versionName: old_version_name ?? 'unknown' }) | ||
| } |
There was a problem hiding this comment.
Normalize old_version_name before uninstall stats.
If old_version_name can include composite suffixes (e.g., 1.2.3:main.js), uninstall stats will fragment by file. Consider stripping the suffix like you already do for version_name.
🛠️ Suggested fix
- if (old_version_name) {
- const oldVersion = await getAppVersionPostgres(c, app_id, old_version_name, undefined, drizzleClient as ReturnType<typeof getDrizzleClient>)
+ if (old_version_name) {
+ const oldVersionOnly = old_version_name.split(':')[0]
+ const oldVersion = await getAppVersionPostgres(c, app_id, oldVersionOnly, undefined, drizzleClient as ReturnType<typeof getDrizzleClient>)
if (oldVersion && oldVersion.id !== appVersion.id) {
- await createStatsVersion(c, old_version_name, app_id, 'uninstall')
- statsActions.push({ action: 'uninstall', versionName: old_version_name ?? 'unknown' })
+ await createStatsVersion(c, oldVersionOnly, app_id, 'uninstall')
+ statsActions.push({ action: 'uninstall', versionName: oldVersionOnly ?? 'unknown' })
}
}📝 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.
| if (action === 'set' && !device.is_emulator && device.is_prod) { | |
| await createStatsVersion(c, appVersion.id, app_id, 'install') | |
| // Use versionOnly (from request body) instead of appVersion - no DB read needed for stats | |
| await createStatsVersion(c, versionOnly, app_id, 'install') | |
| if (old_version_name) { | |
| const oldVersion = await getAppVersionPostgres(c, app_id, old_version_name, undefined, drizzleClient as ReturnType<typeof getDrizzleClient>) | |
| if (oldVersion && oldVersion.id !== appVersion.id) { | |
| await createStatsVersion(c, oldVersion.id, app_id, 'uninstall') | |
| await createStatsVersion(c, old_version_name, app_id, 'uninstall') | |
| statsActions.push({ action: 'uninstall', versionName: old_version_name ?? 'unknown' }) | |
| } | |
| if (action === 'set' && !device.is_emulator && device.is_prod) { | |
| // Use versionOnly (from request body) instead of appVersion - no DB read needed for stats | |
| await createStatsVersion(c, versionOnly, app_id, 'install') | |
| if (old_version_name) { | |
| const oldVersionOnly = old_version_name.split(':')[0] | |
| const oldVersion = await getAppVersionPostgres(c, app_id, oldVersionOnly, undefined, drizzleClient as ReturnType<typeof getDrizzleClient>) | |
| if (oldVersion && oldVersion.id !== appVersion.id) { | |
| await createStatsVersion(c, oldVersionOnly, app_id, 'uninstall') | |
| statsActions.push({ action: 'uninstall', versionName: oldVersionOnly ?? 'unknown' }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@supabase/functions/_backend/plugins/stats.ts` around lines 126 - 134,
Normalize old_version_name the same way version_name/versionOnly is normalized
before using it for uninstall stats: strip any composite suffix (e.g., split on
':' and take the base version) and use that normalized value when calling
getAppVersionPostgres(old_version_name, ...) and createStatsVersion(...,
old_version_name, 'uninstall') and when pushing to statsActions; update the
logic around the old_version_name check in the set branch (where
createStatsVersion, getAppVersionPostgres, appVersion, and statsActions are
used) so all references use the normalized_old_version_name.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/version-name-stats.test.ts`:
- Around line 11-12: Replace usages of BASE_URL in
tests/version-name-stats.test.ts with the routing helper
getEndpointUrl('/triggers/cron_stat_app') so the cron trigger is routed through
the worker helper; update any calls constructing the endpoint (where BASE_URL
and PRODUCT_ID are combined) to use getEndpointUrl instead. Ensure the
Cloudflare Workers testing flag is enabled for this test run (set the test
harness/ENV flag used by your suite so Workers routing is active). Verify
similar replacements for the other occurrences noted (lines ~157-165) to
consistently use getEndpointUrl.
🧹 Nitpick comments (1)
supabase/seed.sql (1)
1006-1007: Consider seeding version_name-only rows to exercise the new path.Right now this still pins a numeric version_id. If the goal is to validate the version_name-only flow during transition, consider inserting NULL for version_id here.
♻️ Optional tweak
- INSERT INTO public.daily_version (date, app_id, version_id, version_name, get, fail, install, uninstall) - VALUES (curr_date, p_app_id, random_version_id, '1.0.0', FLOOR(RANDOM() * 100) + 1, FLOOR(RANDOM() * 10) + 1, FLOOR(RANDOM() * 50) + 1, FLOOR(RANDOM() * 20) + 1); + INSERT INTO public.daily_version (date, app_id, version_id, version_name, get, fail, install, uninstall) + VALUES (curr_date, p_app_id, NULL, '1.0.0', FLOOR(RANDOM() * 100) + 1, FLOOR(RANDOM() * 10) + 1, FLOOR(RANDOM() * 50) + 1, FLOOR(RANDOM() * 20) + 1);
…name Replace version_id (database primary key) with version_name (string) for version statistics tracking in both Cloudflare and Supabase. This eliminates unnecessary database reads during stats collection while maintaining backwards compatibility with existing data via intelligent detection and aggregation. - Update createStatsVersion to accept version_name instead of version_id - Add VersionUsage unified interface for consistent return types - Update Cloudflare trackVersionUsageCF and readStatsVersionCF to use version_name - Update Supabase trackVersionUsageSB and read_version_usage function - Add database migration: adds version_name columns and backfills data - Implement backwards compatibility in cron_stat_app: detect old numeric version_id data and resolve to actual version names with automatic aggregation - Update cron_email to filter by both old and new data formats during transition - Update statistics API and frontend services to use version_name - Add TypeScript type casts for auto-generated stale types until migration runs Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
PostgreSQL doesn't allow changing the return type of an existing function using CREATE OR REPLACE. Must drop the function first, then create with the new signature. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Make version_id nullable (new data won't have it) - Drop old primary key on (date, app_id, version_id) - Add unique constraint on (app_id, date, version_name) for upserts - Set 'unknown' for rows that can't be backfilled - Make version_name NOT NULL after backfill Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
PostgreSQL requires dropping the primary key constraint before making a column nullable if that column is part of the primary key. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
After making version_name NOT NULL in the migration, the seed.sql file needs to include version_name in all daily_version INSERT statements. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Update migration to drop version_usage primary key and make version_id nullable, matching daily_version schema - Add comprehensive test file for version_name statistics tracking: - Tests version_name storage in version_usage - Tests daily_version population via cron_stat_app - Tests read_version_usage function returning version_name - Tests daily_version upsert with version_name constraint - Tests multiple versions with different version_names Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
riderx
force-pushed
the
riderx/stats-version-name
branch
from
2ffeac3 to
66f49d4
Compare
Replace BASE_URL with getEndpointUrl routing helper for the cron_stat_app trigger endpoint, as per coding guidelines for backend tests. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@supabase/functions/_backend/triggers/cron_stat_app.ts`:
- Around line 71-75: Replace the Supabase client lookup for app_versions with
the pg/drizzle pattern: obtain a DB client via getDrizzleClient() (or
getPgClient() if necessary), query the app_versions table in the correct schema
for id and name where id is in versionNamesToResolve, and assign the result to
the versions variable; update any null/undefined checks accordingly (refer to
versions and versionNamesToResolve) and ensure the query uses the schema-aware
table reference for app_versions.
In `@supabase/migrations/20260118005052_version_usage_use_version_name.sql`:
- Around line 22-26: The migration sets NULL version_name to the literal
'unknown' which collapses distinct orphaned version_id rows and will violate the
new unique constraint on (app_id, date, version_name); update the migration to
preserve uniqueness by deriving distinct names (e.g. set version_name =
CONCAT('unknown-', version_id) or another deterministic suffix using the
version_id) for rows where version_name IS NULL, or alternatively
pre-aggregate/merge rows per (app_id, date, version_id) before applying the
UNIQUE constraint; make changes in the UPDATE against the
"public"."daily_version" table and ensure the logic references version_id and
version_name so resulting (app_id, date, version_name) tuples remain unique.
In `@tests/version-name-stats.test.ts`:
- Around line 87-95: The channels seed inserted via
supabase.from('channels').insert(...) is missing the required owner_org field;
update the inserted object in tests/version-name-stats.test.ts (the
supabase.from('channels').insert call) to include an owner_org property with an
appropriate org ID (e.g., the same org used elsewhere in the test setup) so the
channel record conforms to the channels schema and satisfies the pipeline/type
validation.
♻️ Duplicate comments (1)
supabase/functions/_backend/triggers/cron_stat_app.ts (1)
63-68: Numeric-only version names may be remapped as legacy IDs.
If a real version_name is purely numeric, it can be treated as an old ID and resolved to a different name.
🧹 Nitpick comments (1)
tests/version-name-stats.test.ts (1)
24-24: Considerit.concurrentonce per-test isolation is in place.These tests mutate shared rows; to align with the parallel-test guideline, either isolate per test and switch to
it.concurrent, or document why sequential execution is required. As per coding guidelines, ...
| const { data: versions } = await supabase | ||
| .from('app_versions') | ||
| .select('id, name') | ||
| .in('id', versionNamesToResolve) | ||
| if (versions) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use pg/drizzle client for the new app_versions lookup.
This new DB operation should follow the pg/drizzle access pattern mandated for backend functions. As per coding guidelines, use getPgClient() / getDrizzleClient() with schema-based queries.
🤖 Prompt for AI Agents
In `@supabase/functions/_backend/triggers/cron_stat_app.ts` around lines 71 - 75,
Replace the Supabase client lookup for app_versions with the pg/drizzle pattern:
obtain a DB client via getDrizzleClient() (or getPgClient() if necessary), query
the app_versions table in the correct schema for id and name where id is in
versionNamesToResolve, and assign the result to the versions variable; update
any null/undefined checks accordingly (refer to versions and
versionNamesToResolve) and ensure the query uses the schema-aware table
reference for app_versions.
| -- 3b. Set 'unknown' for any rows that couldn't be backfilled (deleted versions) | ||
| UPDATE "public"."daily_version" | ||
| SET version_name = 'unknown' | ||
| WHERE version_name IS NULL; | ||
|
|
There was a problem hiding this comment.
Avoid collapsing multiple orphaned version_ids into a single unknown key.
Setting all NULL version_name values to 'unknown' can create duplicate (app_id, date, version_name) rows and cause the new unique constraint to fail. Consider preserving uniqueness by suffixing the orphaned version_id (or pre-aggregating) before adding the constraint.
🐛 Suggested migration adjustment
-UPDATE "public"."daily_version"
-SET version_name = 'unknown'
-WHERE version_name IS NULL;
+UPDATE "public"."daily_version"
+SET version_name = CONCAT('unknown:', version_id)
+WHERE version_name IS NULL AND version_id IS NOT NULL;
+
+UPDATE "public"."daily_version"
+SET version_name = 'unknown'
+WHERE version_name IS NULL;Also applies to: 84-86
🤖 Prompt for AI Agents
In `@supabase/migrations/20260118005052_version_usage_use_version_name.sql` around
lines 22 - 26, The migration sets NULL version_name to the literal 'unknown'
which collapses distinct orphaned version_id rows and will violate the new
unique constraint on (app_id, date, version_name); update the migration to
preserve uniqueness by deriving distinct names (e.g. set version_name =
CONCAT('unknown-', version_id) or another deterministic suffix using the
version_id) for rows where version_name IS NULL, or alternatively
pre-aggregate/merge rows per (app_id, date, version_id) before applying the
UNIQUE constraint; make changes in the UPDATE against the
"public"."daily_version" table and ensure the logic references version_id and
version_name so resulting (app_id, date, version_name) tuples remain unique.
- Add version_name column to version_usage table types (nullable) - Add version_name column to daily_version table types (required) - Make version_id nullable in both tables - Update read_version_usage return type to use version_name - Fix channels insert in test to include required owner_org - Remove unused eslint-disable directive in cron_stat_app - Remove unnecessary type casts now that types are updated Updates types in both src/types and supabase/functions/_backend/utils. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Summary
Migrate version statistics from using
version_id(database primary key) toversion_name(string). This eliminates unnecessary database reads during stats collection by using the version name already available from the request body, while maintaining full backwards compatibility with existing Cloudflare Analytics Engine data through intelligent detection and aggregation.Test plan
bun test tests/bundle-usage.unit.test.tsto verify bundle usage calculationsbun test tests/statistics.test.tsto verify statistics endpoints with both auth methodscron_email.tscorrectly filters stats for both old and new data formatsChecklist
bun run lint:backendbunx tsc --noEmit)🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.