feat(target-postgres): class-flow migration authoring#359
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a class-flow migration IR ( Changes
Sequence Diagram(s)sequenceDiagram
participant Planner
participant Call as OpFactoryCall
participant Renderer
participant ToOp as toOp()
participant RuntimeOp as SqlMigrationPlanOperation
participant TS as TypeScriptOutput
Planner->>Call: construct *Call(...) and append to calls[]
Planner->>Renderer: renderCallsToTypeScript(calls, meta)
loop per call
Renderer->>Call: call.importRequirements()
Renderer->>Call: call.renderTypeScript()
end
Renderer->>TS: emit imports + class scaffold + operations body
Planner->>Call: renderOps(calls)
loop per call
Call->>ToOp: call.toOp()
ToOp->>RuntimeOp: produce SqlMigrationPlanOperation
end
RuntimeOp-->>Planner: operations[]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 unit tests (beta)
Comment |
@prisma-next/mongo-runtime
@prisma-next/family-mongo
@prisma-next/sql-runtime
@prisma-next/family-sql
@prisma-next/middleware-telemetry
@prisma-next/mongo
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/postgres
@prisma-next/sql-orm-client
@prisma-next/sqlite
@prisma-next/target-mongo
@prisma-next/adapter-mongo
@prisma-next/driver-mongo
@prisma-next/contract
@prisma-next/utils
@prisma-next/config
@prisma-next/errors
@prisma-next/framework-components
@prisma-next/operations
@prisma-next/ts-render
@prisma-next/contract-authoring
@prisma-next/ids
@prisma-next/psl-parser
@prisma-next/psl-printer
@prisma-next/cli
@prisma-next/emitter
@prisma-next/migration-tools
prisma-next
@prisma-next/vite-plugin-contract-emit
@prisma-next/runtime-executor
@prisma-next/mongo-codec
@prisma-next/mongo-contract
@prisma-next/mongo-value
@prisma-next/mongo-contract-psl
@prisma-next/mongo-contract-ts
@prisma-next/mongo-emitter
@prisma-next/mongo-schema-ir
@prisma-next/mongo-query-ast
@prisma-next/mongo-orm
@prisma-next/mongo-query-builder
@prisma-next/mongo-lowering
@prisma-next/mongo-wire
@prisma-next/sql-contract
@prisma-next/sql-errors
@prisma-next/sql-operations
@prisma-next/sql-schema-ir
@prisma-next/sql-contract-psl
@prisma-next/sql-contract-ts
@prisma-next/sql-contract-emitter
@prisma-next/sql-lane-query-builder
@prisma-next/sql-relational-core
@prisma-next/sql-builder
@prisma-next/target-postgres
@prisma-next/target-sqlite
@prisma-next/adapter-postgres
@prisma-next/adapter-sqlite
@prisma-next/driver-postgres
@prisma-next/driver-sqlite
commit: |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
wmadden
commented
Apr 21, 2026
wmadden
added a commit
that referenced
this pull request
Apr 21, 2026
…hifts Updates the project spec, phase-1 spec, plan, and PR plan to match the shipped code after the PR #359 review addressed F05/W02/W04/W05/W07/W10 and related findings. Preserves the original phase-1 design narrative with a post-implementation addendum at the top that names the divergences: - OpFactoryCall.factory is now factoryName (W02) - TsExpression / ImportRequirement / jsonToTsSource live in a shared @prisma-next/framework-ts-render package, consumed by both targets (W04+W05); per-target MigrationTsExpression duplicates are gone - DataTransformCall accepts checkSlot/runSlot strings directly, renders () => placeholder("slot") at its call site, and always throws PN-MIG-2001 from toOp() (W07 + simplification) - Lowering calls to runtime ops is polymorphic via abstract toOp(); the PostgresOpFactoryCallVisitor interface and renderOps visitor are gone (W10) - Pure factories live under operations/{tables,columns,...}.ts (W06) - Factory inventory gained rawSql, createExtension, createSchema plus a liftOpToCall dispatcher (F05/F06) Adds a follow-up for the family-API retarget (Path A) that eliminates the liftOpToCall bridge before SQLite lands, and consolidates the cross-target renderable-migration follow-up around the remaining target-specific TypeScriptRenderable*Migration concrete classes.
wmadden
force-pushed
the
tml-2286-backport-class-flow-migration-authoring-to-the-postgres
branch
from
cd73e18 to
e38c0c5
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts (1)
300-318:⚠️ Potential issue | 🟠 MajorThe success path still drops the authoring IR.
materializePlanItems(items)lowers everyPostgresOpFactoryCallinto runtime ops, and the returned plan still implementsrenderTypeScript()by throwing. That means non-empty Postgres plans still cannot serialize back tomigration.ts, because the only authoring representation is discarded before it reaches callers. The newTypeScriptRenderablePostgresMigrationpath never becomes reachable fromplan().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts` around lines 300 - 318, The plan currently calls materializePlanItems(items) which discards the authoring IR and causes renderTypeScript() to throw; instead preserve the original factory-call items and return a TypeScript-renderable plan when authoring is possible: stop replacing items with materializePlanItems(items) in createMigrationPlan, keep the original items (or store both materialized ops and the original items) and implement renderTypeScript() to delegate to the TypeScriptRenderablePostgresMigration path (or construct/return a TypeScriptRenderablePostgresMigration using the original PostgresOpFactoryCall items); update the plan returned from plan() / createMigrationPlan to include that renderable behavior instead of throwing.
🧹 Nitpick comments (2)
packages/3-targets/3-targets/postgres/src/core/migrations/operation-resolver.ts (1)
235-244: UseifDefined()for conditional object spreads.The conditional spread pattern on lines 242-243 should use
ifDefined()per coding guidelines. The utility is already imported on line 22.As per coding guidelines: "Use
ifDefined()from@prisma-next/utils/definedfor conditional object spreads instead of inline conditional spread patterns"♻️ Proposed fix
const spec: ForeignKeySpec = { name: fkName, columns: fk.columns, references: { table: fk.references.table, columns: fk.references.columns, }, - ...(fk.onDelete !== undefined && { onDelete: fk.onDelete }), - ...(fk.onUpdate !== undefined && { onUpdate: fk.onUpdate }), + ...ifDefined('onDelete', fk.onDelete), + ...ifDefined('onUpdate', fk.onUpdate), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/3-targets/postgres/src/core/migrations/operation-resolver.ts` around lines 235 - 244, The ForeignKeySpec object uses inline conditional spreads for onDelete/onUpdate; replace those spreads with ifDefined() calls (imported from `@prisma-next/utils/defined`) when building the spec in the operation-resolver: update the construction of spec (const spec: ForeignKeySpec) to include ...(ifDefined({ onDelete: fk.onDelete })) and ...(ifDefined({ onUpdate: fk.onUpdate })) instead of the current ...(fk.onDelete !== undefined && { onDelete: fk.onDelete }) / ...(fk.onUpdate !== undefined && { onUpdate: fk.onUpdate }) patterns so the conditional properties are added via ifDefined().packages/3-targets/3-targets/postgres/src/core/migrations/op-factories.ts (1)
9-10: Documentation inconsistency with actual imports.The header comment states "factories never import
@prisma-next/framework-components" but lines 17-20 do importDataTransformOperationandSerializedQueryPlanfrom that package. Consider updating the comment to reflect the actual (and necessary) dependency forcreateDataTransform.Also applies to: 17-20
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/3-targets/postgres/src/core/migrations/op-factories.ts` around lines 9 - 10, The file header incorrectly claims factories never import `@prisma-next/framework-components` while this file imports DataTransformOperation and SerializedQueryPlan and uses createDataTransform; update the top-of-file comment to reflect that Phase 0 factories may import those specific types (DataTransformOperation, SerializedQueryPlan) or remove the blanket prohibition and mention the allowed dependency for createDataTransform so the doc matches the actual imports and usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/operation-resolver.ts`:
- Line 397: The import for qualifyName is placed after function definitions in
operation-resolver.ts; move the "qualifyName" import from
'@prisma-next/adapter-postgres/control' into the top import block with the other
imports (or remove it if qualifyName is already imported earlier), and ensure
any usages of qualifyName still resolve to the single top-level import so there
are no duplicate or late-scope imports referencing the function.
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/render-typescript.ts`:
- Around line 12-15: The file render-typescript.ts under src/core imports
migration-plane symbols (detectScaffoldRuntime, shebangLineFor from
`@prisma-next/migration-tools/migration-ts`) which violates the core vs
migration-plane split; move the scaffold emitter logic out of src/core into a
migration-plane module (e.g., src/exports/control.ts or a new file under
src/exports) so core no longer imports migration-ts. Specifically, extract the
code in render-typescript.ts that uses detectScaffoldRuntime and shebangLineFor
(and any functions that call renderLiteral or PostgresOpFactoryCall rendering
tied to migration output) into the new migration-plane file, update imports
there to `@prisma-next/migration-tools/migration-ts`, and leave only plane-neutral
helpers in src/core (or replace the core file with a thin adapter that delegates
to the new export). Ensure package.json and any public export re-exports the new
emitter from src/exports/control.ts.
---
Outside diff comments:
In `@packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts`:
- Around line 300-318: The plan currently calls materializePlanItems(items)
which discards the authoring IR and causes renderTypeScript() to throw; instead
preserve the original factory-call items and return a TypeScript-renderable plan
when authoring is possible: stop replacing items with
materializePlanItems(items) in createMigrationPlan, keep the original items (or
store both materialized ops and the original items) and implement
renderTypeScript() to delegate to the TypeScriptRenderablePostgresMigration path
(or construct/return a TypeScriptRenderablePostgresMigration using the original
PostgresOpFactoryCall items); update the plan returned from plan() /
createMigrationPlan to include that renderable behavior instead of throwing.
---
Nitpick comments:
In `@packages/3-targets/3-targets/postgres/src/core/migrations/op-factories.ts`:
- Around line 9-10: The file header incorrectly claims factories never import
`@prisma-next/framework-components` while this file imports DataTransformOperation
and SerializedQueryPlan and uses createDataTransform; update the top-of-file
comment to reflect that Phase 0 factories may import those specific types
(DataTransformOperation, SerializedQueryPlan) or remove the blanket prohibition
and mention the allowed dependency for createDataTransform so the doc matches
the actual imports and usage.
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/operation-resolver.ts`:
- Around line 235-244: The ForeignKeySpec object uses inline conditional spreads
for onDelete/onUpdate; replace those spreads with ifDefined() calls (imported
from `@prisma-next/utils/defined`) when building the spec in the
operation-resolver: update the construction of spec (const spec: ForeignKeySpec)
to include ...(ifDefined({ onDelete: fk.onDelete })) and ...(ifDefined({
onUpdate: fk.onUpdate })) instead of the current ...(fk.onDelete !== undefined
&& { onDelete: fk.onDelete }) / ...(fk.onUpdate !== undefined && { onUpdate:
fk.onUpdate }) patterns so the conditional properties are added via ifDefined().
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: cd022c4c-1dce-4215-a421-ae9b9f93c955
⛔ Files ignored due to path filters (7)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlprojects/postgres-class-flow-migrations/plan.mdis excluded by!projects/**projects/postgres-class-flow-migrations/pr-plan.mdis excluded by!projects/**projects/postgres-class-flow-migrations/spec.mdis excluded by!projects/**projects/postgres-class-flow-migrations/specs/phase-0-factory-extraction.spec.mdis excluded by!projects/**projects/postgres-class-flow-migrations/specs/phase-1-walk-schema-class-flow.spec.mdis excluded by!projects/**projects/postgres-class-flow-migrations/specs/walk-schema-audit.spec.mdis excluded by!projects/**
📒 Files selected for processing (25)
packages/1-framework/1-core/framework-components/src/control-migration-types.tspackages/1-framework/1-core/framework-components/src/exports/control.tspackages/2-sql/9-family/package.jsonpackages/2-sql/9-family/src/core/sql-migration.tspackages/2-sql/9-family/src/exports/migration.tspackages/2-sql/9-family/tsdown.config.tspackages/3-mongo-target/1-mongo-target/src/core/migration-ts-expression.tspackages/3-mongo-target/1-mongo-target/src/core/op-factory-call.tspackages/3-mongo-target/1-mongo-target/src/core/render-literal.tspackages/3-mongo-target/1-mongo-target/src/core/render-typescript.tspackages/3-targets/3-targets/postgres/package.jsonpackages/3-targets/3-targets/postgres/src/core/migrations/migration-ts-expression.tspackages/3-targets/3-targets/postgres/src/core/migrations/op-factories.tspackages/3-targets/3-targets/postgres/src/core/migrations/op-factory-call.tspackages/3-targets/3-targets/postgres/src/core/migrations/operation-resolver.tspackages/3-targets/3-targets/postgres/src/core/migrations/placeholder-expression.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner-produced-postgres-migration.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner-reconciliation.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner.tspackages/3-targets/3-targets/postgres/src/core/migrations/render-literal.tspackages/3-targets/3-targets/postgres/src/core/migrations/render-ops.tspackages/3-targets/3-targets/postgres/src/core/migrations/render-typescript.tspackages/3-targets/3-targets/postgres/test/migrations/op-factory-call.test.tspackages/3-targets/3-targets/postgres/test/migrations/planner.reconciliation-unit.test.tstest/e2e/framework/test/ddl.test.ts
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/3-targets/3-targets/postgres/src/core/migrations/operation-resolver.ts (1)
405-405: 🛠️ Refactor suggestion | 🟠 MajorMove import to top of file.
This import statement is positioned after function definitions. Move it to the import block at the top of the file with the other imports from
@prisma-next/adapter-postgres/control(line 13).♻️ Proposed fix
-import { qualifyName } from '@prisma-next/adapter-postgres/control';Move to line 13 area and combine with existing import:
-import { createPostgresAdapter } from '@prisma-next/adapter-postgres/adapter'; +import { createPostgresAdapter, qualifyName } from '@prisma-next/adapter-postgres/adapter';Or if
qualifyNamecomes from a different subpath, add it near the existing imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/3-targets/postgres/src/core/migrations/operation-resolver.ts` at line 405, The import "qualifyName" is declared after function definitions; move the import statement for qualifyName from '@prisma-next/adapter-postgres/control' into the top import block with the other imports from that same module (alongside the existing control imports) so all imports are grouped together; update the existing import list to include qualifyName rather than leaving a late import in the body.
🧹 Nitpick comments (4)
packages/2-sql/9-family/src/core/sql-migration.ts (1)
4-24: Consider trimming this long rationale block comment.The class signature already conveys most of this behavior; moving detailed rationale to a short doc note would keep core source leaner.
As per coding guidelines: "Do not add comments if avoidable; prefer code which expresses its intent."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/9-family/src/core/sql-migration.ts` around lines 4 - 24, Shorten the lengthy rationale comment above the SQL migration base class into a concise doc note: remove the multi-paragraph historical rationale and keep a one- or two-sentence summary that states the purpose of the class and its type parameterization (mentioning SqlMigrationPlanOperation and SqlPlanTargetDetails), and note that concrete subclasses like TypeScriptRenderablePostgresMigration set their own targetId while familyId is intentionally omitted; retain only the essential intent, moving the detailed rationale out to external design docs if needed.packages/3-mongo-target/1-mongo-target/test/op-factory-call.test.ts (1)
39-43: Use an object matcher for grouped call metadata assertions.These are related fields on the same object (
factoryName,operationClass,label), so a singletoMatchObjectassertion is preferred and easier to maintain. Please apply the same pattern to similar tests in this file.Example refactor
- expect(call.factoryName).toBe('createIndex'); - expect(call.operationClass).toBe('additive'); - expect(call.label).toBe('Create index on users (email:1)'); + expect(call).toMatchObject({ + factoryName: 'createIndex', + operationClass: 'additive', + label: 'Create index on users (email:1)', + });As per coding guidelines:
**/*.test.ts: “Prefer object matchers (toMatchObject) over multiple individual expect().toBe() calls when checking 2 or more related values in tests.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-mongo-target/1-mongo-target/test/op-factory-call.test.ts` around lines 39 - 43, The assertions on the call object use multiple toBe checks for related properties; replace them with a single object matcher: update the test that inspects call (the variable named call) to assert Object.isFrozen(call) remains and then use expect(call).toMatchObject({ factoryName: 'createIndex', operationClass: 'additive', label: 'Create index on users (email:1)' }); apply the same to other tests in this file that assert two or more related properties on a single object so they use toMatchObject instead of multiple expect(...).toBe calls.packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts (1)
675-688: UseifDefined()for conditional object spreads.Lines 682-683 use inline conditional spread patterns. Per coding guidelines, prefer
ifDefined()from@prisma-next/utils/definedfor consistency.♻️ Proposed fix
const fkSpec: ForeignKeySpec = { name: fkName, columns: foreignKey.columns, references: { table: foreignKey.references.table, columns: foreignKey.references.columns, }, - ...(foreignKey.onDelete !== undefined && { onDelete: foreignKey.onDelete }), - ...(foreignKey.onUpdate !== undefined && { onUpdate: foreignKey.onUpdate }), + ...ifDefined('onDelete', foreignKey.onDelete), + ...ifDefined('onUpdate', foreignKey.onUpdate), };As per coding guidelines: "Use
ifDefined()from@prisma-next/utils/definedfor conditional object spreads instead of inline conditional spread patterns."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts` around lines 675 - 688, The fkSpec object in function that builds foreign keys uses inline conditional spreads for onDelete/onUpdate; replace those spreads with calls to ifDefined() from `@prisma-next/utils/defined` (e.g., set properties via ...ifDefined({ onDelete: foreignKey.onDelete }) and ...ifDefined({ onUpdate: foreignKey.onUpdate })) and add the necessary import for ifDefined; update the construction of fkSpec (the variable named fkSpec and the new AddForeignKeyCall(schemaName, tableName, fkSpec)) so it uses ifDefined() instead of the inline ...(condition && {...}) patterns.packages/3-targets/3-targets/postgres/test/migrations/render-typescript.roundtrip.test.ts (1)
62-103: Consider adding timeout for tsx execution tests.These tests execute generated TypeScript via
tsx, which could be slow depending on the environment. The repo's coding guidelines recommend using timeout helpers from@prisma-next/test-utilsfor TypeScript-related operations.♻️ Suggested addition
+import { timeouts } from '@prisma-next/test-utils'; + describe('TypeScriptRenderablePostgresMigration round-trip', () => { // ... - it('renders TS that re-parses to operations matching renderOps(calls) exactly', async () => { + it('renders TS that re-parses to operations matching renderOps(calls) exactly', { timeout: timeouts.typeScriptCompilation }, async () => {Based on learnings: "In prisma/prisma-next, when writing tests that run TypeScript compilation (e.g., via
tsc --noEmit), avoid hardcoded timeout numbers... useprisma-next/test-utilstimeout helpers."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/3-targets/postgres/test/migrations/render-typescript.roundtrip.test.ts` around lines 62 - 103, The tsx execution in the test (call using tsxPath with execFileAsync) can hang in slow CI; wrap the execFileAsync invocation that runs join(tmpDir, 'migration.ts') (and any readFile of ops.json) with the repo's TypeScript timeout helper from `@prisma-next/test-utils` (or use its provided run-with-timeout utility) and import that helper at the top of the test file; specifically update the section that constructs the migration via TypeScriptRenderablePostgresMigration, writes migration.ts (writeFile) and calls execFileAsync(tsxPath, [...]) to use the timeout helper so the test fails fast instead of hanging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/1-framework/1-core/ts-render/src/index.ts`:
- Around line 1-2: The current src/index.ts is a re-export-only barrel
(exporting JsonObject, JsonValue, jsonToTsSource from json-to-ts-source and
ImportRequirement, TsExpression from ts-expression); move these exports into an
explicit API entrypoint (e.g., create src/exports/index.ts) and relocate the two
export lines there, then remove or turn src/index.ts into a non-barrel file;
alternatively update consumers to import directly from './json-to-ts-source' and
'./ts-expression' and remove the re-exporting symbols (JsonObject, JsonValue,
jsonToTsSource, ImportRequirement, TsExpression) from src/index.ts so the repo
rule is satisfied.
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/operations/columns.ts`:
- Around line 138-176: Both setNotNull and dropNotNull currently return the same
id (`alterNullability.${tableName}.${columnName}`) causing collisions; update
the id string in the setNotNull and dropNotNull functions to be unique (e.g.
include the operation name) such as
`alterNullability.setNotNull.${tableName}.${columnName}` for the function that
sets NOT NULL and `alterNullability.dropNotNull.${tableName}.${columnName}` for
the function that drops NOT NULL so downstream consumers that key by id won’t
collide.
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/operations/enums.ts`:
- Around line 91-106: renameType only prechecks that the source type exists and
misses a precheck that the destination name is free; add a precheck step to the
precheck array for renameType that ensures the target type (toName) does NOT
already exist (e.g., add step(`ensure type "${toName}" does not exist`, /*
negated check of */ enumTypeExistsCheck(schemaName, toName)) or use an existing
enumTypeNotExistsCheck helper), so the operation will fail fast in precheck
rather than during execute; place this new step alongside the existing source
existence precheck and keep the same step/enumType* helper usage patterns.
In
`@packages/3-targets/3-targets/postgres/test/migrations/render-typescript.roundtrip.test.ts`:
- Around line 92-96: The test contains a no-op assertion
`expect(stdout).toContain('')` that always passes; update the test around the
execFileAsync call to either remove that assertion or replace it with a
meaningful check: for example assert `stdout` is empty with
`expect(stdout).toBe('')` if the command should produce no output, or assert it
contains a specific expected string produced by running `${join(tmpDir,
'migration.ts')}` (referencing `execFileAsync`, `tsxPath`, `tmpDir`, and
`stdout`) to verify the migration output. Ensure the new assertion matches the
intended behavior of the executed script.
---
Duplicate comments:
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/operation-resolver.ts`:
- Line 405: The import "qualifyName" is declared after function definitions;
move the import statement for qualifyName from
'@prisma-next/adapter-postgres/control' into the top import block with the other
imports from that same module (alongside the existing control imports) so all
imports are grouped together; update the existing import list to include
qualifyName rather than leaving a late import in the body.
---
Nitpick comments:
In `@packages/2-sql/9-family/src/core/sql-migration.ts`:
- Around line 4-24: Shorten the lengthy rationale comment above the SQL
migration base class into a concise doc note: remove the multi-paragraph
historical rationale and keep a one- or two-sentence summary that states the
purpose of the class and its type parameterization (mentioning
SqlMigrationPlanOperation and SqlPlanTargetDetails), and note that concrete
subclasses like TypeScriptRenderablePostgresMigration set their own targetId
while familyId is intentionally omitted; retain only the essential intent,
moving the detailed rationale out to external design docs if needed.
In `@packages/3-mongo-target/1-mongo-target/test/op-factory-call.test.ts`:
- Around line 39-43: The assertions on the call object use multiple toBe checks
for related properties; replace them with a single object matcher: update the
test that inspects call (the variable named call) to assert
Object.isFrozen(call) remains and then use expect(call).toMatchObject({
factoryName: 'createIndex', operationClass: 'additive', label: 'Create index on
users (email:1)' }); apply the same to other tests in this file that assert two
or more related properties on a single object so they use toMatchObject instead
of multiple expect(...).toBe calls.
In `@packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts`:
- Around line 675-688: The fkSpec object in function that builds foreign keys
uses inline conditional spreads for onDelete/onUpdate; replace those spreads
with calls to ifDefined() from `@prisma-next/utils/defined` (e.g., set properties
via ...ifDefined({ onDelete: foreignKey.onDelete }) and ...ifDefined({ onUpdate:
foreignKey.onUpdate })) and add the necessary import for ifDefined; update the
construction of fkSpec (the variable named fkSpec and the new
AddForeignKeyCall(schemaName, tableName, fkSpec)) so it uses ifDefined() instead
of the inline ...(condition && {...}) patterns.
In
`@packages/3-targets/3-targets/postgres/test/migrations/render-typescript.roundtrip.test.ts`:
- Around line 62-103: The tsx execution in the test (call using tsxPath with
execFileAsync) can hang in slow CI; wrap the execFileAsync invocation that runs
join(tmpDir, 'migration.ts') (and any readFile of ops.json) with the repo's
TypeScript timeout helper from `@prisma-next/test-utils` (or use its provided
run-with-timeout utility) and import that helper at the top of the test file;
specifically update the section that constructs the migration via
TypeScriptRenderablePostgresMigration, writes migration.ts (writeFile) and calls
execFileAsync(tsxPath, [...]) to use the timeout helper so the test fails fast
instead of hanging.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 66c7253e-b160-4f79-abf2-5b1e64d94dec
⛔ Files ignored due to path filters (8)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlprojects/postgres-class-flow-migrations/assets/walk-schema-audit.mdis excluded by!projects/**projects/postgres-class-flow-migrations/plan.mdis excluded by!projects/**projects/postgres-class-flow-migrations/pr-plan.mdis excluded by!projects/**projects/postgres-class-flow-migrations/spec.mdis excluded by!projects/**projects/postgres-class-flow-migrations/specs/phase-0-factory-extraction.spec.mdis excluded by!projects/**projects/postgres-class-flow-migrations/specs/phase-1-walk-schema-class-flow.spec.mdis excluded by!projects/**projects/postgres-class-flow-migrations/specs/walk-schema-audit.spec.mdis excluded by!projects/**
📒 Files selected for processing (49)
packages/1-framework/1-core/framework-components/src/control-migration-types.tspackages/1-framework/1-core/framework-components/src/exports/control.tspackages/1-framework/1-core/ts-render/package.jsonpackages/1-framework/1-core/ts-render/src/index.tspackages/1-framework/1-core/ts-render/src/json-to-ts-source.tspackages/1-framework/1-core/ts-render/src/ts-expression.tspackages/1-framework/1-core/ts-render/tsconfig.jsonpackages/1-framework/1-core/ts-render/tsdown.config.tspackages/1-framework/3-tooling/cli/test/control-api/client.test.tspackages/2-sql/9-family/package.jsonpackages/2-sql/9-family/src/core/migrations/types.tspackages/2-sql/9-family/src/core/sql-migration.tspackages/2-sql/9-family/src/exports/control.tspackages/2-sql/9-family/src/exports/migration.tspackages/2-sql/9-family/tsdown.config.tspackages/3-mongo-target/1-mongo-target/package.jsonpackages/3-mongo-target/1-mongo-target/src/core/op-factory-call.tspackages/3-mongo-target/1-mongo-target/src/core/render-ops.tspackages/3-mongo-target/1-mongo-target/src/core/render-typescript.tspackages/3-mongo-target/1-mongo-target/src/exports/control.tspackages/3-mongo-target/1-mongo-target/test/mongo-planner.test.tspackages/3-mongo-target/1-mongo-target/test/op-factory-call.test.tspackages/3-mongo-target/1-mongo-target/test/render-typescript.test.tspackages/3-targets/3-targets/postgres/package.jsonpackages/3-targets/3-targets/postgres/src/core/migrations/op-factory-call.tspackages/3-targets/3-targets/postgres/src/core/migrations/operation-resolver.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/columns.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/constraints.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/data-transform.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/dependencies.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/enums.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/indexes.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/raw.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/shared.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/tables.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner-produced-postgres-migration.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner-reconciliation.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner.tspackages/3-targets/3-targets/postgres/src/core/migrations/render-ops.tspackages/3-targets/3-targets/postgres/src/core/migrations/render-typescript.tspackages/3-targets/3-targets/postgres/src/exports/migration.tspackages/3-targets/3-targets/postgres/test/migrations/op-factory-call.test.tspackages/3-targets/3-targets/postgres/test/migrations/planner.authoring-surface.test.tspackages/3-targets/3-targets/postgres/test/migrations/planner.reconciliation-unit.test.tspackages/3-targets/3-targets/postgres/test/migrations/planner.reconciliation.issue-to-call.test.tspackages/3-targets/3-targets/postgres/test/migrations/render-typescript.roundtrip.test.tspackages/3-targets/3-targets/postgres/tsdown.config.tstest/e2e/framework/test/ddl.test.tstsconfig.base.json
✅ Files skipped from review due to trivial changes (12)
- packages/2-sql/9-family/src/exports/migration.ts
- packages/2-sql/9-family/tsdown.config.ts
- packages/1-framework/1-core/ts-render/tsconfig.json
- packages/3-mongo-target/1-mongo-target/package.json
- packages/2-sql/9-family/src/core/migrations/types.ts
- tsconfig.base.json
- packages/3-targets/3-targets/postgres/src/core/migrations/operations/raw.ts
- test/e2e/framework/test/ddl.test.ts
- packages/1-framework/1-core/ts-render/package.json
- packages/3-targets/3-targets/postgres/src/exports/migration.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/operations/data-transform.ts
- packages/3-targets/3-targets/postgres/test/migrations/op-factory-call.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/3-targets/3-targets/postgres/package.json
- packages/1-framework/1-core/framework-components/src/exports/control.ts
- packages/2-sql/9-family/package.json
- packages/1-framework/1-core/framework-components/src/control-migration-types.ts
- packages/3-mongo-target/1-mongo-target/src/core/op-factory-call.ts
- packages/3-targets/3-targets/postgres/test/migrations/planner.reconciliation-unit.test.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/planner-produced-postgres-migration.ts
- packages/3-mongo-target/1-mongo-target/src/core/render-typescript.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/render-typescript.ts
wmadden
changed the title
wmadden
added a commit
that referenced
this pull request
Apr 21, 2026
R1: correct walk-schema-audit.md — liftOpToCall unconditionally wraps as RawSqlCall today (not dispatched), and storage-type enum classes are CreateEnumTypeCall / AddEnumValuesCall / DropEnumTypeCall / RenameTypeCall (no CreateTypeCall). Frame structured dispatch as a Phase 4 target. R3: file the ergonomic rawSql(...) helper under plan.md§"Known follow-ups" (Phase 2/3 authoring-surface polish).
wmadden
added a commit
that referenced
this pull request
Apr 21, 2026
Address PR #359 re-review R2: the `unknown` input type on `jsonToTsSource` is a documented ergonomic concession (so structural types whose fields happen to be JSON-compatible can be passed without an index signature). Bound the escape hatch with explicit tests asserting that Symbol, BigInt, and function values throw at runtime — scope-creep guard per the module docstring.
wmadden
added a commit
that referenced
this pull request
Apr 21, 2026
R4: tighten TypeScriptRenderablePostgresMigration docstring to name the `operations`-vs-`renderTypeScript()` asymmetry explicitly — callers who know a plan may carry stubs must not enumerate `operations` before the user has filled them; render to migration.ts and re-load the edited migration instead. Walk-schema never emits stubs today, so this is invisible until the Phase 2 issue-planner integration lands. R5: add a round-trip case for the empty calls list — render → `tsx`-execute → read `ops.json` → `expect([])`. Pins "the empty scaffold executes cleanly" as an invariant, not just "the empty scaffold parses cleanly".
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/3-targets/3-targets/postgres/test/migrations/render-typescript.roundtrip.test.ts (1)
96-96:⚠️ Potential issue | 🟡 MinorReplace the no-op stdout assertion with a meaningful check.
expect(stdout).toContain('')always passes, so it doesn’t verify execution output.Suggested change
- expect(stdout).toContain(''); + expect(stdout).toBe('');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/3-targets/postgres/test/migrations/render-typescript.roundtrip.test.ts` at line 96, The current no-op assertion expect(stdout).toContain('') should be replaced with a real check: assert that stdout is not empty and includes the expected TypeScript output marker; for example replace it by something like expect(stdout).not.toBe('') and expect(stdout).toContain('export') (reference the stdout variable in this test to locate and update the assertion in render-typescript.roundtrip.test.ts).
🧹 Nitpick comments (2)
packages/1-framework/1-core/ts-render/test/json-to-ts-source.test.ts (2)
32-42: Consider table-driving unsupported-type throw casesThese assertions are correct; parameterizing them would reduce duplication and keep future additions simpler.
Optional refactor
- it('throws on Symbol', () => { - expect(() => jsonToTsSource(Symbol('s'))).toThrowError(/unsupported value type "symbol"/); - }); - - it('throws on BigInt', () => { - expect(() => jsonToTsSource(BigInt(1))).toThrowError(/unsupported value type "bigint"/); - }); - - it('throws on function', () => { - expect(() => jsonToTsSource(() => 1)).toThrowError(/unsupported value type "function"/); - }); + it.each([ + ['symbol', Symbol('s')], + ['bigint', BigInt(1)], + ['function', () => 1], + ] as const)('throws on %s', (typeName, value) => { + expect(() => jsonToTsSource(value)).toThrowError( + new RegExp(`unsupported value type "${typeName}"`), + ); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/1-core/ts-render/test/json-to-ts-source.test.ts` around lines 32 - 42, The three duplicate tests asserting jsonToTsSource throws for unsupported types should be table-driven: create an array of cases like [{name: 'Symbol', value: Symbol('s'), regex: /unsupported value type "symbol"/}, ...] and iterate (e.g., cases.forEach or test.each) to generate the it() assertions dynamically, referencing jsonToTsSource and each case's value and regex so future unsupported-type tests can be added by appending to the cases array.
21-23: Add explicit coverage forundefinedobject-property elision
jsonToTsSourcedrops object entries where value isundefined; this behavior is important and worth pinning with a focused assertion to prevent regressions.Proposed test addition
it('renders objects and quotes non-identifier keys', () => { expect(jsonToTsSource({ a: 1, 'weird-key': 2 })).toBe('{ a: 1, "weird-key": 2 }'); }); + + it('omits undefined object properties', () => { + expect(jsonToTsSource({ a: undefined })).toBe('{}'); + expect(jsonToTsSource({ a: 1, b: undefined })).toBe('{ a: 1 }'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/1-framework/1-core/ts-render/test/json-to-ts-source.test.ts` around lines 21 - 23, Add a focused test for jsonToTsSource that asserts object properties with value undefined are elided from the output (e.g., expect(jsonToTsSource({ a: 1, b: undefined })).toBe('{ a: 1 }')); place it near the existing "renders objects and quotes non-identifier keys" test in json-to-ts-source.test.ts so the behavior is pinned and will catch regressions in the jsonToTsSource function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@packages/3-targets/3-targets/postgres/test/migrations/render-typescript.roundtrip.test.ts`:
- Line 96: The current no-op assertion expect(stdout).toContain('') should be
replaced with a real check: assert that stdout is not empty and includes the
expected TypeScript output marker; for example replace it by something like
expect(stdout).not.toBe('') and expect(stdout).toContain('export') (reference
the stdout variable in this test to locate and update the assertion in
render-typescript.roundtrip.test.ts).
---
Nitpick comments:
In `@packages/1-framework/1-core/ts-render/test/json-to-ts-source.test.ts`:
- Around line 32-42: The three duplicate tests asserting jsonToTsSource throws
for unsupported types should be table-driven: create an array of cases like
[{name: 'Symbol', value: Symbol('s'), regex: /unsupported value type "symbol"/},
...] and iterate (e.g., cases.forEach or test.each) to generate the it()
assertions dynamically, referencing jsonToTsSource and each case's value and
regex so future unsupported-type tests can be added by appending to the cases
array.
- Around line 21-23: Add a focused test for jsonToTsSource that asserts object
properties with value undefined are elided from the output (e.g.,
expect(jsonToTsSource({ a: 1, b: undefined })).toBe('{ a: 1 }')); place it near
the existing "renders objects and quotes non-identifier keys" test in
json-to-ts-source.test.ts so the behavior is pinned and will catch regressions
in the jsonToTsSource function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: eac0074e-fd87-4791-88ea-cf9dc2bf3614
⛔ Files ignored due to path filters (2)
projects/postgres-class-flow-migrations/assets/walk-schema-audit.mdis excluded by!projects/**projects/postgres-class-flow-migrations/plan.mdis excluded by!projects/**
📒 Files selected for processing (3)
packages/1-framework/1-core/ts-render/test/json-to-ts-source.test.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner-produced-postgres-migration.tspackages/3-targets/3-targets/postgres/test/migrations/render-typescript.roundtrip.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/3-targets/3-targets/postgres/src/core/migrations/planner-produced-postgres-migration.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/1-framework/1-core/ts-render/README.md`:
- Around line 20-22: The fenced code block containing "jsValue →
codec.encodeJson → JsonValue → jsonToTsSource → TS source text" is
unlabeled and triggers markdownlint MD040; update that fence to include a
language identifier by changing the opening ``` to ```text so the block becomes
a plain-text fenced block (e.g., replace the existing ``` with ```text
immediately before the line starting with "jsValue → ...").
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 48722c71-27a7-4464-a1b6-827584ae87f3
📒 Files selected for processing (5)
packages/1-framework/1-core/ts-render/README.mdpackages/1-framework/1-core/ts-render/biome.jsoncpackages/1-framework/1-core/ts-render/package.jsonpackages/1-framework/1-core/ts-render/tsconfig.prod.jsonpackages/1-framework/1-core/ts-render/vitest.config.ts
✅ Files skipped from review due to trivial changes (3)
- packages/1-framework/1-core/ts-render/biome.jsonc
- packages/1-framework/1-core/ts-render/tsconfig.prod.json
- packages/1-framework/1-core/ts-render/package.json
Contributor
Author
|
Addressing CodeRabbit review #4148577109 (A19a–A19c). All three findings are resolved on the current branch:
|
Contributor
Author
|
Addressing CodeRabbit review #4148637450 (A20a–A20e):
|
Contributor
Author
|
Addressing CodeRabbit review #4148695243 (A21a–A21c):
|
Contributor
Author
|
Addressing CodeRabbit review #4148769335: the single finding (markdownlint MD040 on the unlabelled fenced block in |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/3-targets/3-targets/postgres/test/migrations/op-factory-call.test.ts (2)
456-461: UseifDefined()for the conditional spread helper.The inline
...(table !== undefined ? { table } : {})pattern is against the repo helper convention, and the same pattern repeats below fordetails. Please switch both sites toifDefined()for consistency with the rest of the codebase.As per coding guidelines,
{packages,examples,test}/**/*.{ts,tsx}: "UseifDefined()from@prisma-next/utils/definedfor conditional object spreads instead of inline conditional spread patterns".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/3-targets/postgres/test/migrations/op-factory-call.test.ts` around lines 456 - 461, Replace the inline conditional spread in the schemaObject helper and the similar conditional spread for details with the repository helper ifDefined from `@prisma-next/utils/defined`; specifically, in the schemaObject arrow function (symbol: schemaObject) replace ...(table !== undefined ? { table } : {}) with the ifDefined(table, t => ({ table: t })) usage, and likewise update the details construction to use ifDefined(...) instead of the inline conditional spread so both sites follow the repo convention.
49-623: Split this suite by concern.This file is now 600+ lines and covers four separate behaviors, so failures will get harder to navigate as it grows. Please split it into
op-factory-call.{basic,rendering,render-ops,migration}.test.ts-style files so each stays readable and independently runnable.As per coding guidelines,
**/*.test.ts: "Keep test files under 500 lines..." and "Split test files when they exceed 500 lines, contain multiple distinct concerns, or have multiple top-leveldescribeblocks that can be split by functionality".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/3-targets/postgres/test/migrations/op-factory-call.test.ts` around lines 49 - 623, The test file is too large and mixes multiple concerns; split the suite into separate test files so each focuses on one area: (1) construction/parity tests around CreateTableCall, DataTransformCall, etc. (look for tests referencing CreateTableCall, DataTransformCall and toOp()), (2) renderTypeScript and importRequirements coverage (tests calling renderTypeScript() and importRequirements()), (3) renderOps behavior (the large suite asserting id/operationClass/details for many calls and tests for RawSqlCall and PN-MIG-2001), and (4) the TypeScriptRenderablePostgresMigration tests (tests constructing TypeScriptRenderablePostgresMigration and asserting targetId, destination, describe, renderTypeScript()). Move each group into its own test file, keep the original describe/it blocks intact, and adjust imports/exports so each file imports only the symbols it needs (CreateTableCall, renderCallsToTypeScript, renderOps, TypeScriptRenderablePostgresMigration, RawSqlCall, DataTransformCall, META, etc.).packages/3-targets/3-targets/postgres/src/core/migrations/operation-resolver.ts (1)
257-262: Misleading underscore prefix on used parameter.The
_ctxparameter has an underscore prefix (conventionally indicating an unused parameter), but_ctx.schemaNameis actually used on line 261. Remove the underscore to avoid confusion.♻️ Proposed fix
function resolveDropConstraint( desc: DropConstraintDescriptor, - _ctx: OperationResolverContext, + ctx: OperationResolverContext, ): ResolvedOp { - return createDropConstraint(_ctx.schemaName, desc.table, desc.constraintName); + return createDropConstraint(ctx.schemaName, desc.table, desc.constraintName); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/3-targets/postgres/src/core/migrations/operation-resolver.ts` around lines 257 - 262, The parameter name for resolveDropConstraint currently uses a misleading underscore prefix (_ctx) but is actually read (using _ctx.schemaName); rename the parameter to ctx (or another non-underscore name) in the resolveDropConstraint function signature and update all uses inside that function (e.g., replace _ctx.schemaName with ctx.schemaName) so the parameter correctly reflects that it is used; ensure the function signature still accepts the same type (OperationResolverContext) and that createDropConstraint(ctx.schemaName, desc.table, desc.constraintName) is called.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/operation-resolver.ts`:
- Around line 110-121: The toColumnSpec function currently calls
buildColumnTypeSql without passing storageTypes; update the toColumnSpec
signature to accept storageTypes: Record<string, StorageTypeInstance> and call
buildColumnTypeSql(column, codecHooks, storageTypes), keep building defaultSql
and nullable as before; then update all callers of toColumnSpec (e.g., the
places in operation-resolver/planner that construct column specs) to pass
ctx.toContract.storage.types ?? {} when invoking toColumnSpec so typeRef
resolution works for custom storage types.
In
`@packages/3-targets/3-targets/postgres/test/migrations/op-factory-call.test.ts`:
- Around line 352-379: Replace the loose import assertion with an
exact-duplication check so the test verifies there is exactly one merged
`@prisma-next/target-postgres/migration` import line produced by
renderCallsToTypeScript(META); locate the test block (op-factory-call.test.ts)
and change the expectation that currently checks for the merged import with
toContain(...) into a single-occurrence assertion (e.g., assert the rendered
source contains the exact merged import string and that its occurrence count is
1 using a regex match against the import text) so any additional stray imports
cause the test to fail.
---
Nitpick comments:
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/operation-resolver.ts`:
- Around line 257-262: The parameter name for resolveDropConstraint currently
uses a misleading underscore prefix (_ctx) but is actually read (using
_ctx.schemaName); rename the parameter to ctx (or another non-underscore name)
in the resolveDropConstraint function signature and update all uses inside that
function (e.g., replace _ctx.schemaName with ctx.schemaName) so the parameter
correctly reflects that it is used; ensure the function signature still accepts
the same type (OperationResolverContext) and that
createDropConstraint(ctx.schemaName, desc.table, desc.constraintName) is called.
In
`@packages/3-targets/3-targets/postgres/test/migrations/op-factory-call.test.ts`:
- Around line 456-461: Replace the inline conditional spread in the schemaObject
helper and the similar conditional spread for details with the repository helper
ifDefined from `@prisma-next/utils/defined`; specifically, in the schemaObject
arrow function (symbol: schemaObject) replace ...(table !== undefined ? { table
} : {}) with the ifDefined(table, t => ({ table: t })) usage, and likewise
update the details construction to use ifDefined(...) instead of the inline
conditional spread so both sites follow the repo convention.
- Around line 49-623: The test file is too large and mixes multiple concerns;
split the suite into separate test files so each focuses on one area: (1)
construction/parity tests around CreateTableCall, DataTransformCall, etc. (look
for tests referencing CreateTableCall, DataTransformCall and toOp()), (2)
renderTypeScript and importRequirements coverage (tests calling
renderTypeScript() and importRequirements()), (3) renderOps behavior (the large
suite asserting id/operationClass/details for many calls and tests for
RawSqlCall and PN-MIG-2001), and (4) the TypeScriptRenderablePostgresMigration
tests (tests constructing TypeScriptRenderablePostgresMigration and asserting
targetId, destination, describe, renderTypeScript()). Move each group into its
own test file, keep the original describe/it blocks intact, and adjust
imports/exports so each file imports only the symbols it needs (CreateTableCall,
renderCallsToTypeScript, renderOps, TypeScriptRenderablePostgresMigration,
RawSqlCall, DataTransformCall, META, etc.).
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 2f5813d9-2fbd-468a-9094-ffe5cc446e94
📒 Files selected for processing (11)
packages/1-framework/1-core/ts-render/README.mdpackages/1-framework/1-core/ts-render/test/json-to-ts-source.test.tspackages/3-mongo-target/1-mongo-target/test/op-factory-call.test.tspackages/3-targets/3-targets/postgres/src/core/migrations/operation-resolver.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/columns.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/enums.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner.tspackages/3-targets/3-targets/postgres/test/migrations/op-factory-call.test.tspackages/3-targets/3-targets/postgres/test/migrations/planner.reconciliation-unit.test.tspackages/3-targets/3-targets/postgres/test/migrations/planner.reconciliation.test.tspackages/3-targets/3-targets/postgres/test/migrations/render-typescript.roundtrip.test.ts
✅ Files skipped from review due to trivial changes (4)
- packages/1-framework/1-core/ts-render/test/json-to-ts-source.test.ts
- packages/3-mongo-target/1-mongo-target/test/op-factory-call.test.ts
- packages/3-targets/3-targets/postgres/test/migrations/render-typescript.roundtrip.test.ts
- packages/1-framework/1-core/ts-render/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/3-targets/3-targets/postgres/test/migrations/planner.reconciliation-unit.test.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/operations/enums.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/operations/columns.ts
…tical goal in favor of runner post-apply verify
…mpl + exports/ barrel
…ionTsExpression AST nodes
Replace the placeholder-closure-with-hidden-symbol design with a proper
Postgres-internal AST hierarchy:
- MigrationTsExpression (abstract, package-private): renderTypeScript() +
importRequirements(). Root of everything the TypeScript renderer walks.
- PostgresOpFactoryCallNode extends MigrationTsExpression and implements
OpFactoryCall; adds visitor accept() for renderOps.
- PlaceholderExpression extends MigrationTsExpression; renders as
() => placeholder("slot") and declares the placeholder import.
DataTransformCall holds check/run as MigrationTsExpression (broad, to leave
room for future variants like a ClosureExpression). renderCallsToTypeScript
becomes polymorphic (not a visitor), walking nodes via renderTypeScript() /
importRequirements() and deduplicating imports. renderOps stays a visitor;
its dataTransform case routes check/run through a bodyToClosure() helper
that returns () => placeholder(slot) for PlaceholderExpression.
Option B for placeholder-bearing plans: migration plan writes migration.ts
only. ops.json and migration.json are re-produced when the user runs the
filled-in migration.ts directly. Captured as new R2.11 in spec.md and as
CLI catch behavior in plan.md Phase 3.
Phase 0 shrinks accordingly - no placeholderClosure helpers are added to
@prisma-next/errors/migration; Phase 0 is pure factory extraction only.
… hierarchy in Phase 1 Mongo and Postgres ship the same IR shape. Phase 1 now adds a Mongo-internal MigrationTsExpression (sibling of the Postgres one), makes OpFactoryCallNode extend it, gives each concrete call class polymorphic renderTypeScript() + importRequirements() methods, and rewrites renderCallsToTypeScript as a polymorphic walk. Byte-identical Mongo migration.ts output; existing snapshot tests are the regression gate. No Mongo planner / factory / CLI changes. Cross-target lift of MigrationTsExpression + ImportRequirement + TypeScriptRenderableMigration remains a known follow-up; Phase 1 sync keeps the two hierarchies byte-compatible at lift time.
… coherence After the recent MigrationTsExpression + Option B placeholder redesign and the Mongo IR sync addition, several stale references remained across the project docs. Reconcile them: - plan.md: fix the end-state directory diagram so TypeScriptRenderable- PostgresMigration lives in planner-produced-postgres-migration.ts (mirroring the Phase 1 spec), and note planner.ts becomes a thin shell post-Phase 4. - plan.md: expand the Mongo IR sync scope to mention the render-literal.ts helper extraction (matching the Phase 1 spec), and correct the deleted- helpers list (renderCallVisitor + collectFactoryNames; buildImports stays). - plan.md, pr-plan.md: point the walk-schema audit location at projects/postgres-class-flow-migrations/assets/walk-schema-audit.md in all places (audit spec is the authoritative source). - pr-plan.md: drop the claim that PR 1 introduces MigrationPlanWithAuthoring- Surface. That interface already exists from ADR 194 / the Mongo work; only OpFactoryCall is new. - spec.md: tighten the Step 3 acceptance criterion so renderOps and renderCallsToTypeScript are described as package-internal (not exported from the user-facing /migration subpath, which continues to expose only the pure factories per R3.1). - spec.md: rewrite the Type-safe-exhaustiveness non-functional requirement so it correctly describes the two dispatch mechanisms (renderOps visitor + polymorphic MigrationTsExpression methods) instead of claiming both renderers use a visitor. - phase-1-walk-schema-class-flow.spec.md: stop mislabeling the walk-schema audit as "the Phase 0 audit" - it is produced as the first research task of Phase 1.
…-SQL Migration alias Phase 1 foundation (PR1): first small, independent slice that unblocks subsequent IR work in both Postgres and Mongo. - framework-components/control: add OpFactoryCall interface and re-export it alongside the other migration types. Interface-only; no abstract base at framework or family level. - target-mongo: OpFactoryCallNode now implements the framework OpFactoryCall so consumers can depend on the framework interface uniformly. - family-sql: add SqlMigration<TDetails> alias (extends Migration<SqlMigrationPlanOperation<TDetails>>) and expose it via a new /migration subpath barrel, mirroring family-mongo's layout. Adds @prisma-next/migration-tools as a dependency and wires the new entry through tsdown + package.json exports. All existing target-mongo tests pass unchanged.
R1: correct walk-schema-audit.md — liftOpToCall unconditionally wraps as RawSqlCall today (not dispatched), and storage-type enum classes are CreateEnumTypeCall / AddEnumValuesCall / DropEnumTypeCall / RenameTypeCall (no CreateTypeCall). Frame structured dispatch as a Phase 4 target. R3: file the ergonomic rawSql(...) helper under plan.md§"Known follow-ups" (Phase 2/3 authoring-surface polish).
Address PR #359 re-review R2: the `unknown` input type on `jsonToTsSource` is a documented ergonomic concession (so structural types whose fields happen to be JSON-compatible can be passed without an index signature). Bound the escape hatch with explicit tests asserting that Symbol, BigInt, and function values throw at runtime — scope-creep guard per the module docstring.
R4: tighten TypeScriptRenderablePostgresMigration docstring to name the `operations`-vs-`renderTypeScript()` asymmetry explicitly — callers who know a plan may carry stubs must not enumerate `operations` before the user has filled them; render to migration.ts and re-load the edited migration instead. Walk-schema never emits stubs today, so this is invisible until the Phase 2 issue-planner integration lands. R5: add a round-trip case for the empty calls list — render → `tsx`-execute → read `ops.json` → `expect([])`. Pins "the empty scaffold executes cleanly" as an invariant, not just "the empty scaffold parses cleanly".
Add tsconfig.prod.json, biome.jsonc, and vitest.config.ts to align @prisma-next/ts-render with the 1-core sibling package layout. The shared @prisma-next/tsdown preset expects tsconfig.prod.json — the CI build was failing with "ERROR tsconfig tsconfig.prod.json doesnt exist" (locally: WARN) and exiting non-zero despite the rollup build succeeding.
Fill the remaining gaps against packages/1-framework/1-core/errors and /operations: - Add README.md (internal-package boilerplate plus a short overview of TsExpression, jsonToTsSource, and the codec → TS pipeline that @prisma-next/ts-render is the second stage of). - Add a `test:coverage` script; drop `--passWithNoTests` from `test` now that the package ships real unit coverage. File-set now matches the other core framework utility packages exactly (biome.jsonc, package.json, README.md, tsconfig.json, tsconfig.prod.json, tsdown.config.ts, vitest.config.ts).
…unreachable ?? fallback in mongo render-typescript Adds json-to-ts-source tests for the two multi-line branches (arrays and objects exceeding 80 chars on a single line) and for the `__proto__` key quoting branch in renderKey, bringing json-to-ts-source.ts to 100% coverage. Also replaces an unreachable `?? []` fallback on a Map.get() in render-typescript with a non-null assertion: we iterate the map keys immediately before, so the branch can never be taken. Keeps 1-mongo-target at 100% branch coverage per the prefer-assertions-over-defensive-checks rule.
…eachable fallback biome forbids non-null assertions, and the previous `?? []` fallback on `symbolsByModule.get()` was unreachable (we iterated the map keys immediately before). Iterate the Map directly with destructuring instead — insertion order is stable, and no factory ever produces more than one non-base module anyway, so sort ordering was never exercised. Keeps 1-mongo-target at 100% branch/statement/function coverage and clears the biome `noNonNullAssertion` lint warning.
Every test in sqlite-driver.test.ts creates a real SQLite database file
and issues multiple queries against it, which can exceed 100ms of cold
I/O on a loaded CI runner. Intermittent timeouts surfaced as:
Error: Test timed out in 200ms.
`timeouts.default` (100ms × multiplier) is the wrong budget for
disk-backed DB work. Switch `testTimeout` and `hookTimeout` to
`timeouts.databaseOperation` (5s × multiplier), which is what the
test-utils API explicitly defines for database queries, setup, and
teardown. Tests still complete in tens of milliseconds locally.
wmadden
force-pushed
the
tml-2286-backport-class-flow-migration-authoring-to-the-postgres
branch
from
e22820d to
975e63c
Compare
…ry-call.test.ts The deduplicates+sorts-imports test used toContain() which only proved the merged import line existed, not that it was the only one. Filter for lines matching @prisma-next/target-postgres/migration and assert equality against a single-element array so a regression that emits a stray duplicate import would fail the test.
… op-factory-call.test.ts
Replace inline `...(x !== undefined ? { x } : {})` patterns in the
schemaObject helper and the renderOps expectation loop with
`ifDefined` from @prisma-next/utils/defined, per use-if-defined.mdc.
…t (it is used) The _ prefix convention signals an unused parameter, but resolveDropConstraint reads ctx.schemaName. Rename the parameter + usage site to match the convention used elsewhere in this file.
Contributor
Author
|
Addressed review #359 (review) (A06a): replaced the two inline conditional spread sites in |
Contributor
Author
|
Addressed review #359 (review) (A06c): renamed |
wmadden
added a commit
that referenced
this pull request
Apr 21, 2026
…hifts Updates the project spec, phase-1 spec, plan, and PR plan to match the shipped code after the PR #359 review addressed F05/W02/W04/W05/W07/W10 and related findings. Preserves the original phase-1 design narrative with a post-implementation addendum at the top that names the divergences: - OpFactoryCall.factory is now factoryName (W02) - TsExpression / ImportRequirement / jsonToTsSource live in a shared @prisma-next/framework-ts-render package, consumed by both targets (W04+W05); per-target MigrationTsExpression duplicates are gone - DataTransformCall accepts checkSlot/runSlot strings directly, renders () => placeholder("slot") at its call site, and always throws PN-MIG-2001 from toOp() (W07 + simplification) - Lowering calls to runtime ops is polymorphic via abstract toOp(); the PostgresOpFactoryCallVisitor interface and renderOps visitor are gone (W10) - Pure factories live under operations/{tables,columns,...}.ts (W06) - Factory inventory gained rawSql, createExtension, createSchema plus a liftOpToCall dispatcher (F05/F06) Adds a follow-up for the family-API retarget (Path A) that eliminates the liftOpToCall bridge before SQLite lands, and consolidates the cross-target renderable-migration follow-up around the remaining target-specific TypeScriptRenderable*Migration concrete classes.
wmadden
added a commit
that referenced
this pull request
Apr 21, 2026
R1: correct walk-schema-audit.md — liftOpToCall unconditionally wraps as RawSqlCall today (not dispatched), and storage-type enum classes are CreateEnumTypeCall / AddEnumValuesCall / DropEnumTypeCall / RenameTypeCall (no CreateTypeCall). Frame structured dispatch as a Phase 4 target. R3: file the ergonomic rawSql(...) helper under plan.md§"Known follow-ups" (Phase 2/3 authoring-surface polish).
wmadden
deleted the
tml-2286-backport-class-flow-migration-authoring-to-the-postgres
branch
wmadden
added a commit
that referenced
this pull request
Apr 21, 2026
Address PR #359 re-review R2: the `unknown` input type on `jsonToTsSource` is a documented ergonomic concession (so structural types whose fields happen to be JSON-compatible can be passed without an index signature). Bound the escape hatch with explicit tests asserting that Symbol, BigInt, and function values throw at runtime — scope-creep guard per the module docstring.
wmadden
added a commit
that referenced
this pull request
Apr 21, 2026
R4: tighten TypeScriptRenderablePostgresMigration docstring to name the `operations`-vs-`renderTypeScript()` asymmetry explicitly — callers who know a plan may carry stubs must not enumerate `operations` before the user has filled them; render to migration.ts and re-load the edited migration instead. Walk-schema never emits stubs today, so this is invisible until the Phase 2 issue-planner integration lands. R5: add a round-trip case for the empty calls list — render → `tsx`-execute → read `ops.json` → `expect([])`. Pins "the empty scaffold executes cleanly" as an invariant, not just "the empty scaffold parses cleanly".
This was referenced Apr 22, 2026
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
2 participants
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.
closes TML-2286
Intent
Bring the Postgres target up to the class-flow migration authoring model established for Mongo in ADR 193 / 194 / 195. The planner's output becomes a single thing — a
TypeScriptRenderablePostgresMigrationthat carriesreadonly PostgresOpFactoryCall[]and exposes both views:get operations(runtime ops, via each call'stoOp()) andrenderTypeScript()(themigration.tsscaffold). Walk-schema (the planner behinddb update) is retargeted to emit the new IR; Mongo is brought to structural parity so the sharedTsExpression/jsonToTsSourcemachinery can live in one framework package.No user-visible CLI change. The issue planner (
migration plan), the CLI scaffolder,migrationStrategy, and Mongo's planner/CLI wiring are untouched — they remain scope for later phases.Change map
@prisma-next/ts-render.TsExpressionabstract base +ImportRequirement+jsonToTsSource(pure JSON-to-TS printer). Single source of truth shared by both targets.Migrationalias.@prisma-next/family-sql/migrationnow re-exportsSqlMigration<TDetails extends SqlPlanTargetDetails>. Authored migrations writeclass M extends Migration { … }; concrete target subclasses extend the same alias.operation-resolver.tsis sliced into context-free factories underpackages/3-targets/3-targets/postgres/src/core/migrations/operations/{tables,columns,constraints,indexes,enums,dependencies,raw,data-transform}.ts. Each factory takes literal args and returns oneSqlMigrationPlanOperation.*Callclass per factory (CreateTableCall,AddColumnCall, …, plusRawSqlCall,CreateExtensionCall,CreateSchemaCall,DataTransformCall) underop-factory-call.ts. Each class is frozen, carries its literal args, and implementstoOp(),renderTypeScript(), andimportRequirements()polymorphically.renderOps(calls) = calls.map(c => c.toOp());renderCallsToTypeScript(calls, meta)walks the list polymorphically and aggregates imports per-module. No visitor boilerplate.planSqlaccumulates areadonly PostgresOpFactoryCall[]and returnsTypeScriptRenderablePostgresMigration.planner-reconciliation.tsemits call classes perSchemaIssuekind. Pre-materialized ops from codec hooks and component database dependencies are laundered at the boundary through aRawSqlCallwrapper so the public output stays uniform.TsExpressionbase, same polymorphictoOp(), no visitor. Byte-identical to pre-change rendered output.projects/postgres-class-flow-migrations/carries the spec, plan, phase specs, a walk-schema audit (every walk-schema branch →SchemaIssuekind → absorbable-by-Phase-4 verdict), and the PR review artifacts.Before / After (planner return shape)
Before:
After:
Behavior changes & evidence
A. New
@prisma-next/ts-renderframework packagePure JSON-to-TS printing + the
TsExpressionbase. The printer accepts JSON-compatible values; non-JSON values (Date, Vector, BigInt) are a codec responsibility — the docstring explicitly names this as the scope boundary.B. Framework exposes
OpFactoryCallas the cross-target IR vocabularyA three-member interface (
factoryName,operationClass,label) any target's class-flow IR satisfies — no abstract base at framework level.C. Postgres pure op factories
Context-free; one op per call; zero dependencies on
@prisma-next/framework-componentsor@prisma-next/contract.operation-resolver.tsis now a thin materialization layer over these.D. Postgres class-flow IR — one call class per factory
Every
*Callclass is frozen at construction, carries literal args, delegates to the matching pure factory intoOp(), and renders itself viajsonToTsSource.DataTransformCallis the planner-stub shape: itstoOp()always throwsPN-MIG-2001(unfilled placeholder) because the planner can only stub, not lower.toOpparity + per-class render + import requirementsE.
TypeScriptRenderablePostgresMigration— planner-produced authoring surfaceHolds
PostgresOpFactoryCall[]+MigrationMeta; implementsget operations,describe(),renderTypeScript(). Extends family-levelSqlMigration<PostgresPlanTargetDetails>, implementsMigrationPlanWithAuthoringSurface.from/tometadata, empty-migration stubF. Walk-schema planner retargeting — uniform
PostgresOpFactoryCall[]outputEvery
buildXmethod returnsreadonly PostgresOpFactoryCall[]. Pre-materialized ops from codec hooks, component database dependencies, and the atomic add-NOT-NULL-with-temporary-default recipe are wrapped inRawSqlCallat the planner boundary so the accumulator is pure. The externalMigrationPlannerResultshape is unchanged; all prior planner integration/behavior/storage-types/reconciliation tests pass unmodified.G. End-to-end authoring-surface invariant
A
tsx-executing round-trip test renders the class-flow TypeScript, writes it to a tempdir, executes it, reads the resultingops.json, and asserts it equalsrenderOps(calls). Also pins byte-preservation forRawSqlCallops through the round trip.H. Mongo structural parity
OpFactoryCallNodeextends the sharedTsExpression; each of the five concrete call classes implementstoOp()/renderTypeScript()polymorphically. The oldrenderCallVisitor,collectFactoryNames, andOpFactoryCallVisitorare deleted. Existing Mongo snapshot / e2e tests are the byte-identity gate.I. Family-SQL
Migrationalias@prisma-next/family-sql/migrationre-exportsSqlMigration<TDetails extends SqlPlanTargetDetails>. Mirrors@prisma-next/family-mongo/migration. Concrete target migration classes extend this alias rather than@prisma-next/migration-tools/migrationdirectly.Compatibility / migration / risk
migration plan,migration new, the CLI scaffolder,migrationStrategy, and capability wiring are untouched. Walk-schema's external op output is preserved byte-for-byte (verified by unchanged integration/behavior tests).DataTransformCall.toOp()throws by design. The planner only stubs data transforms; the user must fill the renderedmigration.tsand re-emit. The walk-schema planner never emitsDataTransformCalltoday, so the throw is reserved for the Phase 2 issue-planner integration.RawSqlCallauthor-facing UX is verbose. Authors writing hand-coded raw SQL migrations must pass a full op shape (id,label,operationClass,target,precheck,execute,postcheck). A lighterrawSql({ sql, label, operationClass })helper is tracked as a Phase 2/3 polish.Follow-ups / open questions
migration plan) to emit class-flow IR. The walk-schema audit underprojects/postgres-class-flow-migrations/assets/walk-schema-audit.mdmaps every walk-schema branch to aSchemaIssuekind and names the handful of issue-union extensions needed to absorb it.migration new.rawSql(...)author-facing helper (see compatibility note above).liftOpToCallas if it dispatched on op shape (it always returnsRawSqlCall), and one references a non-existentCreateTypeCall(should beCreateEnumTypeCall). Cosmetic; tracked in the re-review doc.Non-goals / intentionally out of scope
@prisma-next/ts-renderexists specifically so a future SQLite target reuses the same machinery without another copy landing.Summary by CodeRabbit
New Features
Refactoring
Tests
Documentation