feat(nextly): create a single's table and its registry row in one service - #556
Conversation
|
@codex please review this PR |
|
@coderabbitai full review |
|
@greptileai review |
|
Warning Review limit reached
Next review available in: 40 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 ignored due to path filters (1)
📒 Files selected for processing (15)
📝 WalkthroughWalkthroughChangesSingle metadata and migration flow
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Dispatcher
participant SingleMetadataService
participant SingleRegistryService
participant Database
Dispatcher->>SingleMetadataService: createSingle(input)
SingleMetadataService->>SingleRegistryService: write or adopt pending record
SingleMetadataService->>Database: apply migration statements
Database-->>SingleMetadataService: table state
SingleMetadataService->>SingleRegistryService: record migration status
SingleMetadataService-->>Dispatcher: return record and status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
|
@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: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b33d074730
ℹ️ 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".
|
Pushed
Worth recording that my local check missed it. I was running Re-verified at the new head: @codex please review this PR |
|
@coderabbitai full review |
|
@greptileai review |
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29601b316b
ℹ️ 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".
|
Both Codex P2 findings were real and are fixed in 1. A rejected create was leaving a row behind. My regression: intent-first moved One correction to the report: the throwing branch is gated on 2. Tolerating a re-run could mask a table that does not match. Each fix has an integration test on all three dialects, each proven load-bearing by reverting the fix and confirming that test alone fails. Re-verified: @codex please review this PR |
|
@coderabbitai full review |
|
@greptileai review |
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89de03ae07
ℹ️ 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".
|
Third Codex P2, real, fixed in Repairing a localized entity failed on every dialect, not just MySQL. Reproduced before changing anything, by writing the localized twin of the reapply test and running it against unfixed code: When the registry row is gone, the create path can only describe the entity as brand new, so the plan asks to ADD every translatable column to a companion that already has them. All three companion paths now share one tolerant runner, so half of a localized entity's storage cannot be re-appliable while the other half is not. Re-verified at Running total for this PR: three Codex findings, all three real, all three fixed with a load-bearing cross-dialect test. Two of them were defects this branch introduced; the third was latent and this branch's own repair test is what exposed it. @codex please review this PR |
|
@coderabbitai review |
|
@greptileai review |
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e59147c53
ℹ️ 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".
|
Four more Codex P2s, all real, all fixed in They were four faces of one design mistake I made: I added a tolerance and did not strengthen the verification that the tolerance made necessary. Once So rather than four patches, there is now one shared
Fixing the field-group path also exposed a gap in my own verifier, caught by that dispatcher's unit suite: introspection can THROW, not merely return nothing, and an unreadable catalog would then fail a migration that had succeeded. It now reports nothing when it cannot run — which its docblock already claimed and the code did not do. Three new cross-dialect tests, each proven load-bearing by disabling the check. Running total: seven Codex findings on this PR, all seven real. Verified at @codex please review this PR |
|
@coderabbitai review |
|
@greptileai review |
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b0ba5dead
ℹ️ 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".
0b0ba5d to
74c8820
Compare
…ata service The service is registered in DI, and the registration module is imported at boot by anything touching the container. Importing the schema generators, the runtime-schema builder and the companion reconcile at the top of the file put that whole graph in front of every consumer, including processes that never create a Single. di/register.ts already avoids this for the same three modules.
getService is generic over the container KEY, not the service type, so passing the type failed the typecheck that includes test files. The service was also missing from ServiceMap, which is what forced the explicit argument in the first place: without an entry there a caller has no typed way to resolve it.
…le matches Two ways a create could record a state the database does not have. The DDL generator validates as well as renders — a required relationship declaring onDelete set null is refused there — so generating after the registry row was written let that rejection strand a pending Single with its permissions seeded, and the corrected retry then collided with the slug it had just created. Planning now happens before anything is persisted. CREATE TABLE IF NOT EXISTS is a no-op against a table that already exists, and the index statements after it are tolerated as already applied, so re-running over an orphaned table with a changed field set reported success while the columns were missing. Applied now means the columns are there, not that something of that name is.
…e it Repairing a localized entity whose registry row was lost reaches the reconcile as a brand-new localized entity, because that is all the registry can say. The plan then asks to ADD every translatable column to a companion that already has them, and those statements carry no IF NOT EXISTS on any dialect, so the repair failed on all three rather than only on MySQL. All three companion paths now run their statements through the shared tolerant runner, so half of a localized entity's storage cannot be re-appliable while the other half is not. The matcher still refuses a duplicate row.
Tolerating a re-run removed the loud failure that made a MISMATCHED table obvious. CREATE TABLE IF NOT EXISTS no-ops on every dialect and the index statements after it are now tolerated, so a repair over a table an earlier attempt left behind emitted no error even when the field set, the Draft Published option or a field type had changed. One shared check answers it for all three paths, building the desired shape with the same builder the schema diff compares against, so the system lifecycle columns are included, translatable columns are excluded, and types are compared through the diff engine's own normalizer. A check that cannot run reports nothing, because failing a migration for want of a verification would be worse than the unverified behaviour it replaces.
Writing the intent before touching the database is only worth doing if something can finish it. The row owns the slug the moment it is written, so a create interrupted before it could record its outcome was refused as a duplicate for ever, leaving no way forward short of editing the registry by hand. That is worse than the orphan table this ordering prevents, because an orphan at least left the slug free. A row that is not applied describes an operation that never reported success, so a create naming the same slug adopts it and re-runs the idempotent DDL. The row is re-stated from the new request, because a corrected retry usually differs from the attempt that failed.
A column the Builder now calls optional while the database still has it NOT NULL accepts every write the Builder considers valid and then fails the constraint, at write time and far from the migration that caused it. The desired spec already carried the flag; only the comparison was missing.
The verifier walked only the desired side, so anything the live table still carries that the schema no longer declares was accepted. A removed required field leaves a NOT NULL column that rejects writes omitting it, and a dropped unique leaves a constraint that rejects writes the Builder now allows. A failed create also stopped claiming success. The shape verification makes failed a routine outcome, and the response told the admin to run migrations, which cannot repair a table whose columns do not match.
Adoption treated any non-applied row as resumable, but pending is equally the state of a create running right now. Two overlapping requests for one slug would then have the second overwrite the first's row while its DDL was still building the first schema, after which the original confirms applied against a description that is no longer its own. A failed row is a finished attempt, so taking it over is safe. Serialising the pending case is the migration lock this relocation exists to unblock, and a second timestamp-based exclusion here would leave two of them.
The verifier compared types and index names against the diff engine's ideal schema, but the builder's own generators do not render exactly that, so it failed creates that work: a float number renders decimal while the descriptor says float8, a unique field becomes an inline constraint the database names itself, and the field-group generator emits no created_at index the spec declares for every component. Presence and nullability were measured against every fixture on all three dialects and produce no such false positives, so the check keeps those and drops the rest until the generators and the descriptor agree. The companion also stops tolerating a re-run. A half-finished localization enable reaches that code in the same state as an orphan repair, and the planner cannot tell them apart, so tolerating it would report success while default-locale content is still on the main table. A localized repair now fails loudly instead.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/nextlyhq/nextly/blob/58149acb90166335e018fe8494db2b2967223f4c/packages/nextly/src/domains/schema/services/verify-applied-shape.ts#L182-L187
Align required FK nullability before verifying
For a fresh Single or Component create that contains a required single-target relationship/upload, the direct DDL still emits that column as NOT NULL, while field-column-descriptor reports fkSingle columns as nullable because requiredness is enforced in application code. This new nullability comparison therefore records an otherwise-created table as failed on the normal create path; align the DDL with the descriptor or skip this comparison for those FK columns before using it as the migration outcome gate.
AGENTS.md reference: packages/nextly/AGENTS.md:L30-L32
ℹ️ 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".
Adoption read the failed status and then wrote, so two retries could both see it and both run DDL against one slug. updateSingle also writes migration_status only when the field hash or status flag changes, so the commonest retry of all — the same payload again — left the row saying failed for the whole time its DDL was running. The claim now puts the status in the WHERE clause and marks the row with a token it reads back. Counting returned rows would not do: MySQL has no RETURNING and the adapter re-reads with the same predicate, which the claim has just falsified, so a successful claim reports zero rows there.
58149ac to
95948af
Compare
|
The retry race was narrowed last round, not closed. Now an actual compare-and-swap: the status is in the WHERE clause, so only one caller can match while the row still says 🔴 My first implementation of that CAS was wrong on MySQL, and the test caught it. I counted rows via The MySQL Verified at 24 findings on this PR, 24 real. Zero threads open. @codex please review this PR |
|
@coderabbitai review |
|
@greptileai review |
|
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Codex found a fifth way it fails a working operation: a create with a required The descriptor reports every The full tally:
I narrowed it twice. Each time the false positives moved to whatever comparison remained. That is structural, not a run of oversights: it compares against the diff engine's ideal schema, and the Builder's 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. 🔴 My evidence for keeping it was wrong. I said presence and nullability were "measured safe across every fixture on all three dialects". True of my fixtures — which had no required relationship, no required upload, and no localization enable. The decision to keep those rested on that, so correcting it is mine. The localization case was the worst: marking the migration failed skipped the companion transition while Every path it broke is now a regression test asserting it stays The stale- Verified at 28 findings, 28 real. Zero threads open. @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: ccaa140893
ℹ️ 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".
|
Writing the registry row before the DDL is only better once something can finish an interrupted attempt. Without that, the half-written row owns the slug and refuses every retry — a worse state than the orphan table it prevents, because an orphan at least leaves the slug free. Codex pressed this twice and was right both times; the answer was to stop shipping half a mechanism, not to guess at the other half. So the row is written last with the outcome already known, exactly as the handler did. The planning step stays first, so a request the generator refuses still leaves nothing behind at all — that part was a genuine fix and it survives. Removed with it: the adoption branch, the atomic claim, the relaxed owner check, and their three tests. Nothing is left in the codebase that no longer has a caller. What this PR is now, precisely
It claims nothing it cannot prove. Every behaviour change beyond the relocation was either measured or removed. Verified at 28 findings, 28 real, zero threads open. @codex please review this PR |
|
@coderabbitai review |
|
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Moves the Schema Builder's single create out of the request handler and into a service that
owns the table change and the registry write together. Delete, schema-update and the field-group
side follow in the next two PRs.
Why
A Single's table was created by the handler: generate the DDL, run it, then write the registry row.
Two consequences:
guessing at table names.
tables have already changed. That is what blocks the migration-lock work this belongs to.
Collections already avoid both by owning the halves in one method. This is the same shape for
Singles.
What it guarantees, and what it does not
Recoverable and idempotent. NOT atomic. That is the only honest claim available: MySQL commits
DDL implicitly, so no ordering and no transaction makes the pair atomic there. The migration engine
reached the same conclusion in
field-groups/migration/steps.tsand answers it with "sequenced withrepair rather than atomic, and every half idempotent to make that repair possible". Same answer
here, rather than a second one.
The mechanism is write-ahead intent: the row is persisted as
pendingbefore the table istouched and confirmed afterwards, so an interrupted create leaves a durable record of what was being
attempted and recovery is a query rather than an inference.
That query already existed.
SingleRegistryService.getPendingMigrations()had zero callers,because
pendingnever reached the database —migrationStatuswas a local variable and the rowwas written once, at the end. This connects a design that was already there.
The ordering also moves the registry's own rejections (a taken slug, a name reserved by a global
resource) to before the DDL rather than after it.
The defect this found
🔴 There was no integration test that a
createSinglerequest creates a table. The onlyintegration coverage of the path asserts the REJECTION leaves no orphan. Nothing asserted a
successful create produces one — which is exactly the gap #536 fell through, where a full unit suite
read identical while MySQL tables were not being created at all.
single-create-schema-change.integration.test.tscloses it, driving the real dispatcher against areal database per dialect and asserting the registry row and the physical table, because the
failure being guarded against is the two halves disagreeing.
Its third case re-applies a create over a table it already made — the repair scenario the guarantee
is entirely about:
Duplicate key name 'idx_single_..._created_at'PostgreSQL and SQLite emit
IF NOT EXISTSfor the table and its indexes. MySQL has no such formfor
CREATE INDEX, so the second run died on the index and a correct schema was recorded as afailed migration.
Not introduced here — the handler's bare try/catch behaved identically. What changed is that the
service now claims idempotency, so the claim had to be made true. Each statement now tolerates
isIdempotencyError, the canonical matcher whose own header names MySQL's "Duplicate key name" andwhich deliberately refuses to match a duplicate ROW (error 1062), so a genuine data conflict still
fails.
Notes for review
reconcileSingleCompanionis a pure move, proven two ways rather than one: the function bodydiffs byte-for-byte against
4ce333caemodulo the import-path rewrite, and both forwarding suitesare green either side with identical stderr.
The service's input is
Omit<DynamicSingleInsert, "migrationStatus">, not a hand-listed subset.The first draft listed fields by hand and silently dropped
schemaHash,webhooksand the resolvedversions/revalidateshapes — the create would have written a row missing three settings andnothing in the types or the tests would have said so.
Registered in DI rather than constructed per request so one wrapper governs every caller. The
migration lock has to enclose both halves, and a lock applied at one call site leaves the others
uncovered.
Two test doubles grew; no assertion changed. Both dispatcher suites gained
getSingleMetadataServiceFromDI, wired to a real service over the same registry and adapterdoubles — a stub would leave the assertions describing the stub, since what a request forwards into
the DDL is now decided inside the service. The registry doubles gained
updateMigrationStatus.single-dispatcher-ddl.test.tsalso gainedgetSchemaRegistryFromDI, which silences a warning thatwas already firing on main.
ddlFornow asserts the create's own recordedmigration_status === "applied"first, because acreate that gave up part-way still leaves its earlier statements in
executed— every assertionwould keep passing while describing a run that never finished. Proven load-bearing: forcing
"failed"fails all 6 withexpected 'failed' to be 'applied'.input.fields as unknown as FieldDefinition[]is carried, not introduced. TheFieldConfig/FieldDefinitionboundary is asserted this way at ~10 non-test sites already(
di/register.ts:712,single-query-service.ts:2499,collection-query-service.ts:3106, …).Aligning those two representations is a much larger change than this PR.
Scope, stated rather than quietly narrowed
The plan had this PR covering create, delete, schema-update, the field-group service and all three
field-group transports. It covers create only, for size rather than difficulty:
updateSingleSchemais ~420 lines in the handler andapplySingleSchemaChanges~215, each carryingthe push pipeline, rename detection and the prompt dispatcher. With the field-group side that is
around +2500/−1500 across ~20 files, which is where this repo's review bots start degrading.
FieldGroupMetadataServiceand all three field-group transports, which closes theP1 in task 137, plus converging the companion reconcile.
deleteSingleandupdateSingleSchema, each with its own before/after evidence.Field groups go first because the P1 is live:
api/field-groups.tsanddirect-api/namespaces/field-groups.tsboth write a registry row, never create the table, andreturn
201. Reachable through the publicnextly.fieldGroups.create(). The route already writesmigrationStatus: "pending", so it is intent-first with the apply missing — pointing it at theservice is the fix, with no interim duplication.
Also deliberately not done here: the companion reconcile exists three times (singles,
component-dispatcher.ts:243, inline atcollection-dispatcher.ts:804) with ~45 identical middlelines. A shared
applyCompanionTransitionlands with the field-group PR, which already opens two ofthose files.
One more thing this PR carries
The same MySQL retry dead end existed in every Builder path, not just the one being moved:
single-dispatcherschema-update andcomponent-dispatcherfield-group create both ran statementsthrough a private copy of the splitter with no tolerance. Both now use one shared
applyMigrationStatements, which also removes 2 of the 7 hand-rolled splitters task 136 tracks.CollectionFileManager.runMigrationis deliberately left alone: it executes with per-dialectbranches and swallows its errors, which task 136 already records as a separate, larger decision.
Changing error handling in code that currently hides failures should not happen as a side effect.
Verification
Unit baseline re-measured at
4ce333caein its own installed and built worktree: 363 failures /7275 (main's #544 rewrote
__tests__/fixtures/db.ts; the 397 carried through this programme isstale). Measured twice, stable both times.
Branch: 363 failed / 6869 passed / 7280 — failure set identical to main, zero new, zero
disappeared. The
6869 − 6864 = 5extra passes are exactly the tests this PR adds.🔴 That comparison needs
--testTimeout=60000, and the reason is worth knowing. At the repo's10s default the branch reports 6–14 extra failures, in files this PR does not touch, with a
different set every run. Every one is
Test timed out in 10000ms, never an assertion, and eachpasses in isolation. Re-measuring main on the same machine minutes later returned 0 timeouts twice,
so this is not the environment.
The branch's unit suite runs ~24% slower than main under identical conditions (154s vs 124s),
which is what tips the slowest drizzle-kit-loading files over a 10s limit. The first hypothesis —
that the DI registration module statically importing the schema and i18n machinery defeated the 37
await import()callsdi/register.tsuses for exactly those modules — was tested and refuted;making them lazy changed nothing measurable. The lazy imports are kept because the reasoning stands
on its own, but they are not the cause, and the cause is not yet identified.
If CI shows those files timing out, that is this, not a behaviour change — and the fix is the
timeout or the cause, not the tests.
Build clean ·
tsc --noEmitexit 0 ·lintexit 0 ·check-drizzle-v1-legacyall checks passed ·cross-package (
plugin-sdk,ui,blocks-engine,plugin-seo) 14/14 green.Integration, PostgreSQL 17: 1267 passed, 0 failed.
Integration, MySQL: 1202 passed, 2 failed — and those 2 fail identically on main.
🔴 The MySQL integration baseline is not zero against a shared database.
builder-create-converges.integration.test.tsfails withPUSHSCHEMA_FAILED — resolver(table) was called without a HintsHandlerwheneverTEST_MYSQL_URLpoints at a database holding tables the desired schema does not declare. That suite connects to the
URL directly instead of provisioning its own database, and wires
noopPromptDispatcher. Against adatabase created for the run it passes. Reproduced on
4ce333caewith no branch involved.Worth its own task: the suite should provision its own database like the rest of the harness.
Summary by CodeRabbit
New Features
Bug Fixes