fix(nextly): stop verifying a shape the generators do not produce - #562
Conversation
|
Warning Review limit reached
Next review available in: 5 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change revises schema migration handling and Single creation registration. Rename idempotency errors now propagate. Single and Component creation no longer perform post-migration shape checks. Failed Single migrations preserve prior schema metadata. ChangesSchema migration and creation flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SingleDispatcher
participant SingleMetadataService
participant MigrationRunner
participant SingleRegistryService
SingleDispatcher->>SingleMetadataService: createSingle
SingleMetadataService->>SingleMetadataService: planCreate
SingleMetadataService->>MigrationRunner: Apply migration SQL
MigrationRunner-->>SingleMetadataService: Return migration status
SingleMetadataService->>SingleRegistryService: registerSingle with status
SingleRegistryService-->>SingleMetadataService: Return registered record
SingleMetadataService-->>SingleDispatcher: Return creation result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
@codex please review this PR |
|
@coderabbitai full review |
|
@greptileai review |
|
|
A failed rename is silently swallowed, and the content becomes unreachable
For a CREATE or an ADD, "already exists" means the schema is in the state the statement asked for, which is what makes re-running safe. For a RENAME it means the opposite: the target is occupied, so the move did not happen. Swallowed, the values stay in the old column while the registry records the new field name, and every later read addresses the new column and finds it empty. The user's content is still in the database and no longer reachable. Renames are now excluded from the tolerance. The wording is indistinguishable between the two cases, so the statement is the discriminator rather than the error. Two unit tests, one per wording, proven load-bearing by removing the exclusion. This PR now carries three fixes, all measured against merged main
Verified at @codex please review this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a902f4c89
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
@nextlyhq/adapter-drizzle
@nextlyhq/adapter-mysql
@nextlyhq/adapter-postgres
@nextlyhq/adapter-sqlite
@nextlyhq/admin
@nextlyhq/admin-css
@nextlyhq/blocks-engine
@nextlyhq/blocks-react
create-nextly-app
nextly
@nextlyhq/plugin-form-builder
@nextlyhq/plugin-page-builder
@nextlyhq/plugin-sdk
@nextlyhq/plugin-seo
@nextlyhq/storage-s3
@nextlyhq/storage-uploadthing
@nextlyhq/storage-vercel-blob
@nextlyhq/ui
commit: |
|
FixedP1: a blocked rename still persisted the new field list. My rename fix was necessary and not sufficient. The throw takes the A failed migration no longer persists the new field list or hash. Stated as a rule, not a rename special case: the stored fields are what the runtime is rebuilt from, so a list the table does not have redirects reads whatever caused the failure. 🔴 Applied on reasoning, not a test, and that is on the record. I wrote the integration test, it passed against the broken code, and I deleted it. The probe showed why: dropping My rename regex matched the word anywhere, so a column literally named Rebutted — all three are pre-existing, verified at
|
| finding | evidence |
|---|---|
| a failed create cannot be retried | :655 same owner check, :795 registerSingle last with the final status |
an orphan table plus changed fields records applied |
:708-709 gates on tableExists alone |
| concurrent creates rebind the resolver before the unique insert | same order: DDL → :708 → :740 → :795 |
This PR restores that sequence rather than creating it — the relocation had to keep the order identical so the move could be reviewed as a move.
All three are closed by the migration lock, which is the next task and the thing this relocation exists to unblock: a lock could not enclose these steps before, because one taken inside the registry service is acquired after the tables have already changed.
Verified at f822addc2: build clean · check-types 0 · lint 0 · drizzle gate passed · unit 363 vs 363 against a freshly measured baseline at ccaa14089, zero new · create suite 16 passed on PostgreSQL, 16 on MySQL.
@codex please review this PR
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/nextly/src/dispatcher/handlers/__tests__/single-dispatcher-ddl.test.ts (1)
148-152: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the stale
updateMigrationStatuscomment inwireRegistry.The assertion now reads the status from the first
registerSinglecall. The create path no longer writespendingfirst and no longer callsupdateMigrationStatus. The comment at Lines 97-99 still describes that removed sequence, so it now contradicts both the service and this assertion.Replace it with a statement of why the double is still present, or remove the member if nothing under test calls it.
♻️ Proposed comment update
registerSingle: vi.fn(async (row: unknown) => row), - // The confirm write. A create persists its intent as `pending` before touching the table and - // records the outcome here afterwards, so a double without it fails the whole create. + // Present for the update paths that still record an outcome separately; the create path + // records its status inside the `registerSingle` payload instead. updateMigrationStatus: vi.fn(),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nextly/src/dispatcher/handlers/__tests__/single-dispatcher-ddl.test.ts` around lines 148 - 152, Update the stale updateMigrationStatus comment in wireRegistry to reflect the current create flow and first registerSingle call assertion; explain why the mocked member remains, or remove it if no tested code invokes it.packages/nextly/src/domains/singles/__tests__/single-create-schema-change.integration.test.ts (1)
187-293: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the failed-migration branch that preserves the prior field list.
The three new tests all assert the success path stays
applied. The PR also adds a failure-path behavior insingle-dispatcher.tsat Lines 1117-1129: when a migration fails after it began, the handler dropsfieldsandschemaHashso the registry keeps the previous schema. The removed tests covered failure and resumption scenarios, so that branch now has no integration coverage.Add a test that forces a blocked rename or another post-
migrationBeganfailure, then asserts both facts:
migrationStatusisfailed.- The stored
fieldsstill match the pre-update list.That test would also expose the
localizedandstatusgap raised onsingle-dispatcher.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nextly/src/domains/singles/__tests__/single-create-schema-change.integration.test.ts` around lines 187 - 293, Add an integration test alongside the existing schema migration tests that triggers a post-migrationBegan failure, such as a blocked field rename. Assert the registry row reports migrationStatus as "failed" and that its stored fields remain identical to the pre-update field list, covering the failure handling in the single-dispatcher migration path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/nextly/src/dispatcher/handlers/single-dispatcher.ts`:
- Around line 1117-1129: When migrationStatus is failed, remove
updateData.localized and updateData.status alongside fields and schemaHash
before persistence, preserving the previous runtime schema flags. Update the
failed-migration response in the update path to use the same three-way messaging
behavior as the create path around its existing status message, rather than
reporting that changes are merely pending.
---
Nitpick comments:
In
`@packages/nextly/src/dispatcher/handlers/__tests__/single-dispatcher-ddl.test.ts`:
- Around line 148-152: Update the stale updateMigrationStatus comment in
wireRegistry to reflect the current create flow and first registerSingle call
assertion; explain why the mocked member remains, or remove it if no tested code
invokes it.
In
`@packages/nextly/src/domains/singles/__tests__/single-create-schema-change.integration.test.ts`:
- Around line 187-293: Add an integration test alongside the existing schema
migration tests that triggers a post-migrationBegan failure, such as a blocked
field rename. Assert the registry row reports migrationStatus as "failed" and
that its stored fields remain identical to the pre-update field list, covering
the failure handling in the single-dispatcher migration path.
🪄 Autofix
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.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 287cc942-1238-4230-9e8e-f0fb75f2d2a7
⛔ Files ignored due to path filters (1)
.changeset/builder-schema-change-outcomes.mdis excluded by!.changeset/**
📒 Files selected for processing (9)
packages/nextly/src/dispatcher/handlers/__tests__/single-dispatcher-ddl.test.tspackages/nextly/src/dispatcher/handlers/component-dispatcher.tspackages/nextly/src/dispatcher/handlers/single-dispatcher.tspackages/nextly/src/domains/schema/services/__tests__/apply-migration-statements.test.tspackages/nextly/src/domains/schema/services/apply-migration-statements.tspackages/nextly/src/domains/schema/services/verify-applied-shape.tspackages/nextly/src/domains/singles/__tests__/single-create-schema-change.integration.test.tspackages/nextly/src/domains/singles/services/single-metadata-service.tspackages/nextly/src/domains/singles/services/single-registry-service.ts
💤 Files with no reviewable changes (2)
- packages/nextly/src/domains/schema/services/verify-applied-shape.ts
- packages/nextly/src/domains/singles/services/single-registry-service.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f822addc2f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
f822add to
807c58c
Compare
|
It also led me to a second failure path CodeRabbit did not name, which needs a narrower answer. The companion-transition catch is reached with the main ALTER already applied, so the new The update response no longer calls a failed change "pending" — advice that could not work, and worse now that the field list is not saved either. 🔴 Both fixes are applied on reasoning, not on tests, and that is on the record. I wrote integration tests for each and both passed against the unfixed code, so I deleted them. Probing showed why: my fixtures produce refusals that throw before any statement runs, which persist nothing. Reaching a genuinely part-applied failure needs one statement to succeed and a later one to fail, which I could not construct through the public API — and that is probably why these paths went unnoticed in the first place. Rebased onto @codex please review this PR |
|
@coderabbitai review |
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 807c58caf5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
My previous fix opened a hole. The retry tolerance is now opt-in, and only the create paths ask for it. A duplicate On the orphan component table, one correction to my own earlier framing. It is mostly pre-existing — The guard being asked for in two of these is the post-apply shape check, which was built and removed on #556 because it failed five working operations — including every field-group create, which is this exact path. It would have refused every create it was meant to protect. Verified at @codex please review this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ac22b51fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
0ac22b5 to
c267482
Compare
|
The P1: my withholding was in the wrong catch. There are two companion catches. The P2: I removed the 🔴 That is the third finding in this family, so the general point is worth stating: a part-applied migration leaves no registry value that is correct in every case. Withholding is right where one direction is provably safer and wrong where it is not. What would settle it is knowing which statements reached the database — the migration journal's job, not this handler's. Rebased onto @codex please review this PR |
|
Three things had to line up, and all three did: the catch set the status on the update payload but not the local; I also fixed the success branch, which was the same defect pointing the other way: a flag-only change that worked reported whatever the row said before, so a Single already at 🔴 Third finding in a row where the withholding logic was right and wired to something that never saw it. The shape of it: the migration status is derived in four places and consumed in two, and nothing checks that a producer reaches its consumer. Recorded rather than restructured at the end of this PR. Verified at @codex please review this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61c5aa6a5d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
A repair could never clear its own failure. Round 20 taught the local variable about success but left the write gated on The running server disagreed with the row it had just persisted. Withholding the localization flag without touching the resolver left this process resolving the Single through the schema the save was attempting. Re-registered with the state that will actually be saved. Withholding fields on a companion failure: answered, not fixed. It is the same symmetric case as 🔴 The pattern, stated plainlyRounds 18, 19, 20 and 22 were all the same shape: the logic was right and the wiring reached something that never saw it — the wrong catch, the wrong flag, the wrong variable, the wrong gate. The cause is structural, not carelessness: the migration status is derived in six places and consumed in two, and nothing checks that a producer reaches its consumer. Every incremental fix in this area has spawned the next finding, which is exactly what a fan-out with no convergence point produces. I am not restructuring that at the end of this PR. It is recorded as the thing to fix before anyone adds a seventh producer, and as the reason the part-applied-migration class needs the migration journal rather than more handler logic. Verified at @codex please review this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 780bc51d8d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…k-types The response was read before the follow-up status write, so whenever that write was the only thing to change the status the body carried the previous value — most visibly reporting a successful repair as still failed. Also adds the builder argument a test merged from an older branch was missing. check-types fails on main itself without it, which blocks every PR.
780bc51 to
474334c
Compare
🔴 #556 merged at
95948afb3, two commits behind the branch head. The two commits that were notincluded are the ones that REMOVE a check which fails working operations, so main currently carries
two live regressions. This PR is those two commits, rebased onto main.
Proven on merged main, not argued
Ran the branch's own regression guards against a worktree at
ccaa14089, PostgreSQL and SQLite:1. A create with a required relationship or upload is recorded as
failedfield-column-descriptor.ts:230reports everyfkSinglecolumn as nullable on purpose —requiredness is enforced in application code — while the create DDL emits
NOT NULL. Thenullability comparison therefore fails a table that was created correctly.
2. Enabling localization is recorded as
failedAND strands contentThis is the serious one. A field moving to the companion is legitimately still a
NOT NULLcolumnon the main table until the companion transition seeds and drops it. The check runs first, sees
columns "no longer declared" — including the system
titleandslug— marks the migration failed,and skips the companion transition while
localized: truehas already been persisted. Readsthen point at an unseeded companion while the content is still on main.
A verification that strands data is worse than no verification.
Why removal rather than another narrowing
The check was narrowed twice already, dropping type and then index comparison. Each time the false
positives moved to whatever remained. The cause is structural: it compares against the diff engine's
ideal schema, and the Builder's own generators do not render that. Those divergences are real
and already tracked as their own work; a post-apply gate is not where they should be discovered.
Measured tally over the review of #556:
Also included: the write ordering returns to recording the outcome with the row
Writing the intent first is better only once something can finish an interrupted attempt. Without
that, the half-written row owns the slug and refuses every retry — worse than the orphan table it
prevents, because an orphan at least leaves the slug free. The ordering moves with the migration
lock that completes it.
Removed with it: the adoption branch, the atomic claim and the relaxed owner check. Nothing without
a caller is left behind.
Kept: planning before anything is persisted, so a request the generator refuses leaves nothing
at all. That was a genuine fix.
What #556's work still delivers after this
The relocation itself (−356 lines from the handler), the extracted companion reconcile, the shared
statement runner that fixes the MySQL retry dead end, and 445 lines of integration tests proving a
create actually creates a table on all three dialects.
Verification
Build clean ·
check-types0 ·lint0 · drizzle gate passed · the create suite 16 passed onPostgreSQL, 16 on MySQL · dispatcher suites back to the main baseline exactly.
The three cases the check broke are kept as regression tests asserting they stay
applied.Summary by CodeRabbit
Bug Fixes
New Features
Tests