Fix #30: Support args#31
Conversation
📝 WalkthroughWalkthroughThis pull request implements runtime migration arguments support for the Convex migrations library. Changes include typed argument definition and inference utilities, per-args migration tracking to enable independent runs, argument propagation through migration flows, and updates to configuration and documentation demonstrating the feature. Changes
Sequence DiagramsequenceDiagram
participant User
participant Client as Migrations.define
participant Core as Migrations.runOne
participant Component as Component Lib
participant Schema as Migrations Table
User->>Client: define(table, { args: validator, migrateOne(..., args) })
Client->>Client: InferMigrationArgs from validator
User->>Core: runOne(fnRef, { args: {...} })
Core->>Core: ExtractMigrationArgs from fnRef
Core->>Core: migrationNameWithArgs(name, args)
Core->>Component: runMutation with typed args
Component->>Component: Generate unique name from args
Component->>Schema: Store args alongside migration entry
Component->>Component: Load next migration with matching (name, args)
Component->>Component: Call migrateOne(query, args)
Component->>Component: Call customRange(query, args)
Component->>Schema: Update documents
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tsconfig.test.json (1)
3-8: Missing path alias for@convex-dev/migrations/test.The
vitest.config.tsdefines an alias for@convex-dev/migrations/test→src/test.ts, but this tsconfig doesn't include it. This could cause TypeScript type-checking errors when importing from that subpath in test files.🔧 Proposed fix to add the missing alias
"compilerOptions": { "baseUrl": ".", "paths": { - "@convex-dev/migrations": ["./src/client/index.ts"] + "@convex-dev/migrations": ["./src/client/index.ts"], + "@convex-dev/migrations/test": ["./src/test.ts"] } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tsconfig.test.json` around lines 3 - 8, tsconfig.test.json is missing the path alias for the test subpath used by vitest; add a paths entry mapping "@convex-dev/migrations/test" to the test module (e.g., "src/test.ts") alongside the existing "@convex-dev/migrations" mapping so TypeScript resolves imports from "@convex-dev/migrations/test" during test type-checking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/index.ts`:
- Around line 735-737: The current logic lets opts.name (nameOverride) bypass
arg-based namespacing so different args can reuse the same migration state;
change the naming so args are always incorporated: compute a baseName =
nameOverride ?? getFunctionName(fnRef) and then call
migrationNameWithArgs(baseName, args) to produce name (so when callers pass
opts.name plus args they still get arg-scoped migration names), referencing
opts.name/nameOverride, getFunctionName(fnRef), migrationNameWithArgs, and args.
- Around line 202-205: The code passes the synthetic migration key `name` (from
`migrationNameWithArgs(baseName, args.args)`) into `makeFn`, but `makeFn` calls
`makeFunctionReference<"mutation">(fn)` and needs the real function path; change
the argument passed to `makeFn` to the real function name (use `baseName` or
`getFunctionName(fnRef!)` instead of `name`). Also preserve the original args
when mapping follow-up migrations: in the block handling `args.next` (the
mapping at lines 231-237), include the `args` field on each mapped object so
follow-up migrations keep their args context. Ensure changes reference `makeFn`,
`makeFunctionReference`, `baseName`, `name`, `migrationNameWithArgs`, and
`args.next`.
In `@src/component/lib.ts`:
- Around line 366-372: The migrationNameWithArgs function currently uses
JSON.stringify with a top-level replacer which fails for nested objects; replace
that with a deep stable serializer: add a helper (e.g., stableStringify or
sortObjectDeep) that recursively walks values, sorts object keys at every level,
preserves arrays order and primitive types, and returns a deterministic
structure which you then JSON.stringify to create the name in
migrationNameWithArgs; ensure it handles null, Date/RegExp/primitive values
appropriately (or serializes them predictably) so
migrationNameWithArgs(baseName, args) returns `${baseName}(${stableString})`
with stableString produced by that recursive key-sorting serializer.
---
Nitpick comments:
In `@tsconfig.test.json`:
- Around line 3-8: tsconfig.test.json is missing the path alias for the test
subpath used by vitest; add a paths entry mapping "@convex-dev/migrations/test"
to the test module (e.g., "src/test.ts") alongside the existing
"@convex-dev/migrations" mapping so TypeScript resolves imports from
"@convex-dev/migrations/test" during test type-checking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d66fa5d-3ba8-43bc-9ba0-57ca273d10ef
⛔ Files ignored due to path filters (1)
src/component/_generated/component.tsis excluded by!**/_generated/**
📒 Files selected for processing (11)
README.mdconvex.jsonexample/convex/example.test.tsexample/convex/example.tsexample/convex/tsconfig.jsonsrc/client/index.tssrc/component/lib.tssrc/component/schema.tssrc/shared.tstsconfig.test.jsonvitest.config.ts
| const baseName = args.fn | ||
| ? this.prefixedName(args.fn) | ||
| : getFunctionName(fnRef!); | ||
| const name = migrationNameWithArgs(baseName, args.args); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's examine the code around lines 202-205 and 219-228 in src/client/index.ts
head -n 240 src/client/index.ts | tail -n 50 | cat -nRepository: get-convex/migrations
Length of output: 1917
🏁 Script executed:
#!/bin/bash
# Get a clearer view of the specific lines mentioned in the review
sed -n '195,235p' src/client/index.ts | cat -nRepository: get-convex/migrations
Length of output: 1620
🏁 Script executed:
#!/bin/bash
# Let's also find the function containing these lines and understand the context
rg -n "_runInteractive|runToCompletion" src/client/index.ts -A 5 -B 5Repository: get-convex/migrations
Length of output: 2181
🏁 Script executed:
#!/bin/bash
# Search for makeFn definition and usage patterns
rg -n "makeFn|await createFunctionHandle" src/client/index.tsRepository: get-convex/migrations
Length of output: 606
Pass real function name to makeFn, not the synthetic migration state key.
Line 220 passes name (a synthetic migration state key created from baseName and args.args) to makeFn, but makeFn calls makeFunctionReference<"mutation">(fn) which expects a real Convex function path. This will fail with "Can't find function" at runtime.
Additionally, when processing args.next at lines 231-237, the mapped objects omit the args field. Follow-up migrations scheduled via args.next will lose the original args context.
Suggested fix
const fnHandle = args.fn
- ? await makeFn(name)
+ ? await makeFn(baseName)
: await createFunctionHandle(fnRef!);
@@
await Promise.all(
args.next.map(async (nextFn) => ({
name: this.prefixedName(nextFn),
fnHandle: await makeFn(this.prefixedName(nextFn)),
+ args: args.args,
})),
),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/index.ts` around lines 202 - 205, The code passes the synthetic
migration key `name` (from `migrationNameWithArgs(baseName, args.args)`) into
`makeFn`, but `makeFn` calls `makeFunctionReference<"mutation">(fn)` and needs
the real function path; change the argument passed to `makeFn` to the real
function name (use `baseName` or `getFunctionName(fnRef!)` instead of `name`).
Also preserve the original args when mapping follow-up migrations: in the block
handling `args.next` (the mapping at lines 231-237), include the `args` field on
each mapped object so follow-up migrations keep their args context. Ensure
changes reference `makeFn`, `makeFunctionReference`, `baseName`, `name`,
`migrationNameWithArgs`, and `args.next`.
| const { name: nameOverride, batchSize, dryRun = false, args } = opts ?? {}; | ||
| const name = | ||
| nameOverride ?? migrationNameWithArgs(getFunctionName(fnRef), args); |
There was a problem hiding this comment.
Don't let opts.name bypass arg-based namespacing.
Handle-based callers are told to pass name: getFunctionName(...). With args present, this branch keeps that raw base name, so two runs with different args will reuse the same migration state and the second one can no-op incorrectly.
Possible fix
- const name =
- nameOverride ?? migrationNameWithArgs(getFunctionName(fnRef), args);
+ const baseName = nameOverride ?? getFunctionName(fnRef);
+ const name = migrationNameWithArgs(baseName, args);📝 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.
| const { name: nameOverride, batchSize, dryRun = false, args } = opts ?? {}; | |
| const name = | |
| nameOverride ?? migrationNameWithArgs(getFunctionName(fnRef), args); | |
| const { name: nameOverride, batchSize, dryRun = false, args } = opts ?? {}; | |
| const baseName = nameOverride ?? getFunctionName(fnRef); | |
| const name = migrationNameWithArgs(baseName, args); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/index.ts` around lines 735 - 737, The current logic lets opts.name
(nameOverride) bypass arg-based namespacing so different args can reuse the same
migration state; change the naming so args are always incorporated: compute a
baseName = nameOverride ?? getFunctionName(fnRef) and then call
migrationNameWithArgs(baseName, args) to produce name (so when callers pass
opts.name plus args they still get arg-scoped migration names), referencing
opts.name/nameOverride, getFunctionName(fnRef), migrationNameWithArgs, and args.
| export function migrationNameWithArgs( | ||
| baseName: string, | ||
| args?: unknown, | ||
| ): string { | ||
| if (args === undefined || args === null) return baseName; | ||
| const serialized = JSON.stringify(args, Object.keys(args as any).sort()); | ||
| return `${baseName}(${serialized})`; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file and code location
cd src/component && wc -l lib.tsRepository: get-convex/migrations
Length of output: 75
🏁 Script executed:
# Read the relevant section
sed -n '360,380p' src/component/lib.tsRepository: get-convex/migrations
Length of output: 504
🏁 Script executed:
# Check how migrationNameWithArgs is used in the codebase
rg "migrationNameWithArgs" -A 2 -B 2Repository: get-convex/migrations
Length of output: 2206
🏁 Script executed:
# Test JSON.stringify behavior with replacer array
node <<'EOF'
// Test 1: Basic nested object with replacer array containing only top-level keys
const args = { name: "migration", config: { deep: "value" } };
const topLevelKeys = Object.keys(args).sort();
console.log("=== Test 1: Nested object with top-level key replacer ===");
console.log("Input:", JSON.stringify(args));
console.log("Keys:", topLevelKeys);
console.log("Result:", JSON.stringify(args, topLevelKeys));
console.log();
// Test 2: Multiple nested objects with different deep structures
const args2 = { name: "test", nested: { x: 1, y: 2 } };
const args3 = { name: "test", nested: { x: 1, y: 3 } };
const keys2 = Object.keys(args2).sort();
console.log("=== Test 2: Different nested structures, same top-level keys ===");
console.log("Args2:", JSON.stringify(args2, keys2));
console.log("Args3:", JSON.stringify(args3, keys2));
console.log("Are they equal?", JSON.stringify(args2, keys2) === JSON.stringify(args3, keys2));
console.log();
// Test 3: Demonstrate the actual collision scenario
const migrationArgsA = { table: "users", version: 1, schema: { id: "int", name: "string" } };
const migrationArgsB = { table: "users", version: 1, schema: { id: "text", name: "text" } };
const topKeys = Object.keys(migrationArgsA).sort();
console.log("=== Test 3: Schema collision scenario ===");
console.log("MigrationA:", JSON.stringify(migrationArgsA, topKeys));
console.log("MigrationB:", JSON.stringify(migrationArgsB, topKeys));
console.log("Both produce:", JSON.stringify(migrationArgsA, topKeys));
EOFRepository: get-convex/migrations
Length of output: 608
🏁 Script executed:
# Test the proposed stableSort solution
node <<'EOF'
function stableSort(value) {
if (Array.isArray(value)) return value.map(stableSort);
if (value && typeof value === "object") {
return Object.fromEntries(
Object.entries(value)
.sort(([a], [b]) => a.localeCompare(b))
.map(([key, nested]) => [key, stableSort(nested)]),
);
}
return value;
}
// Test 1: Nested objects with different deep structures
const migrationArgsA = { table: "users", version: 1, schema: { id: "int", name: "string" } };
const migrationArgsB = { table: "users", version: 1, schema: { id: "text", name: "text" } };
console.log("=== Proposed stableSort Solution ===");
console.log("A with stableSort:", JSON.stringify(stableSort(migrationArgsA)));
console.log("B with stableSort:", JSON.stringify(stableSort(migrationArgsB)));
console.log("Are they different?", JSON.stringify(stableSort(migrationArgsA)) !== JSON.stringify(stableSort(migrationArgsB)));
console.log();
// Test 2: Verify determinism with key ordering
const argsKeyOrder1 = { z: 1, a: 2, m: 3 };
const argsKeyOrder2 = { a: 2, m: 3, z: 1 };
console.log("=== Determinism test (different key order) ===");
console.log("Order 1:", JSON.stringify(stableSort(argsKeyOrder1)));
console.log("Order 2:", JSON.stringify(stableSort(argsKeyOrder2)));
console.log("Are they identical?", JSON.stringify(stableSort(argsKeyOrder1)) === JSON.stringify(stableSort(argsKeyOrder2)));
console.log();
// Test 3: Edge cases
console.log("=== Edge cases ===");
console.log("Array:", JSON.stringify(stableSort([3, 1, 2])));
console.log("Null:", JSON.stringify(stableSort(null)));
console.log("Undefined:", JSON.stringify(stableSort(undefined)));
console.log("Primitive:", JSON.stringify(stableSort(42)));
EOFRepository: get-convex/migrations
Length of output: 511
Use a deep stable serializer for arg-derived names.
The current implementation using JSON.stringify(args, Object.keys(args as any).sort()) has a critical flaw: the replacer array parameter only includes top-level keys, so nested objects are serialized as empty objects {}. This causes migrations with different nested args to produce identical names and incorrectly share state.
For example, { table: "users", schema: { id: "int" } } and { table: "users", schema: { id: "text" } } both serialize to {"schema":{},"table":"users"}.
Replace with a recursive function that sorts keys at all nesting levels:
Recommended fix
export function migrationNameWithArgs(
baseName: string,
args?: unknown,
): string {
if (args === undefined || args === null) return baseName;
- const serialized = JSON.stringify(args, Object.keys(args as any).sort());
+ const serialized = JSON.stringify(stableSort(args));
return `${baseName}(${serialized})`;
}
+
+function stableSort(value: unknown): unknown {
+ if (Array.isArray(value)) return value.map(stableSort);
+ if (value && typeof value === "object") {
+ return Object.fromEntries(
+ Object.entries(value as Record<string, unknown>)
+ .sort(([a], [b]) => a.localeCompare(b))
+ .map(([key, nested]) => [key, stableSort(nested)]),
+ );
+ }
+ return value;
+}📝 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.
| export function migrationNameWithArgs( | |
| baseName: string, | |
| args?: unknown, | |
| ): string { | |
| if (args === undefined || args === null) return baseName; | |
| const serialized = JSON.stringify(args, Object.keys(args as any).sort()); | |
| return `${baseName}(${serialized})`; | |
| export function migrationNameWithArgs( | |
| baseName: string, | |
| args?: unknown, | |
| ): string { | |
| if (args === undefined || args === null) return baseName; | |
| const serialized = JSON.stringify(stableSort(args)); | |
| return `${baseName}(${serialized})`; | |
| } | |
| function stableSort(value: unknown): unknown { | |
| if (Array.isArray(value)) return value.map(stableSort); | |
| if (value && typeof value === "object") { | |
| return Object.fromEntries( | |
| Object.entries(value as Record<string, unknown>) | |
| .sort(([a], [b]) => a.localeCompare(b)) | |
| .map(([key, nested]) => [key, stableSort(nested)]), | |
| ); | |
| } | |
| return value; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/component/lib.ts` around lines 366 - 372, The migrationNameWithArgs
function currently uses JSON.stringify with a top-level replacer which fails for
nested objects; replace that with a deep stable serializer: add a helper (e.g.,
stableStringify or sortObjectDeep) that recursively walks values, sorts object
keys at every level, preserves arrays order and primitive types, and returns a
deterministic structure which you then JSON.stringify to create the name in
migrationNameWithArgs; ensure it handles null, Date/RegExp/primitive values
appropriately (or serializes them predictably) so
migrationNameWithArgs(baseName, args) returns `${baseName}(${stableString})`
with stableString produced by that recursive key-sorting serializer.
commit: |
Have you tried running the mutation directly? The runner isn't strictly necessary when running from the dashboard / CLI. You just need to pass in a And soon I want to add |
| "paths": { | ||
| "@convex-dev/migrations": ["./src/client/index.ts"] | ||
| } |
There was a problem hiding this comment.
I don't think this should be necessary, since the top-level package.json has the package name and entrypoint - were you seeing something off?
There was a problem hiding this comment.
Without this I got typescript errors when checking out the repo and opening it in Cursor (on CLI as well I think).
| // This sucks but import.meta.url error in TypeScript without a way to fix it | ||
| // and __dirname is not available in ESM. This relies on where the tests are run from. | ||
| const root = resolve(process.cwd()); | ||
|
|
||
| export default defineConfig({ | ||
| resolve: { | ||
| alias: { | ||
| // More specific alias first so "@convex-dev/migrations/test" matches correctly | ||
| "@convex-dev/migrations/test": resolve(root, "src/test.ts"), | ||
| "@convex-dev/migrations": resolve(root, "src/client/index.ts"), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
I haven't needed to do this before - why was this necessary? And iirc import.meta.dirname works in these configs?
There was a problem hiding this comment.
Try it out yourself, not sure why I'd get different results to you.
I find this very inconvenient for the devs (who are onboarding onto Convex and barely understand it). |
|
Agreed! I want to add either |
2 participants
I realized that this kinda moves the migrations towards workflows, and I wonder whether the migrations as implemented are even the right abstraction. It feels like "I want to run something durably over an index range" should be separated from "I want to run a thing I'll call 'foo' once". Anyway, the main motivator for this is cleaning up production DB with 10K records as we're deploying it. I think that should be really implemented in the product.
How we use it:
I also don't love the need to wrap in runner to just run one migration.
I used Claude but also fixed things myself after testing, but tests, examples, README could probably be improved.
Fixes #30
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.