feat(nextly): resolve field-group storage names from the catalog - #454
Conversation
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (33)
📝 WalkthroughWalkthroughThe PR resolves legacy and migrated field-group registry tables and discriminator columns from live catalog metadata. It updates schema registration, runtime operations, queries, CLI commands, reload handling, migration cache invalidation, and regression tests. ChangesField-group storage resolution
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant StorageNameResolver
participant Catalog
participant RuntimeSchemaService
participant SchemaRegistry
Caller->>StorageNameResolver: resolve registry and discriminator names
StorageNameResolver->>Catalog: read tables, columns, and identifier rules
Catalog-->>StorageNameResolver: return live storage metadata
StorageNameResolver-->>Caller: return physical names
Caller->>RuntimeSchemaService: generate schema with resolved typeColumn
RuntimeSchemaService-->>Caller: return dialect schema
Caller->>SchemaRegistry: register schema and registry aliases
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 22 minutes. |
@nextlyhq/adapter-drizzle
@nextlyhq/adapter-mysql
@nextlyhq/adapter-postgres
@nextlyhq/adapter-sqlite
@nextlyhq/admin
@nextlyhq/admin-css
@nextlyhq/blocks-engine
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: |
cb89a17 to
10ebc85
Compare
|
Rebased onto main
No unresolved threads. CodeRabbit is rate limited and has not actually reviewed — its note says so even though its check reports pass. Not merged. |
|
@codex please review this PR |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 15 minutes. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10ebc85413
ℹ️ 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".
|
Round update, head
The pattern I got wrong, and it is worth stating plainly: I swept for the places that read field-group storage and stopped there. Four of these six are writes and deletes, and three of them fail silently because the surrounding code already treats that failure as benign. Resolution has to cover everything that addresses the storage, not everything that reads it — and a swallowed error is not a smaller bug than a thrown one, it is a larger one. Finding 5 needed a structural change worth reviewing: the value condition was built once per filter, above the per-table loop. That cannot be right when the column name is per table, since a dynamic-zone filter spans several tables the migration may have moved independently. The switch is now a module-level helper called inside the loop. Same logic, evaluated per table. Verification: All six threads replied and resolved. Not merged. |
|
@codex please review this PR |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 46 minutes. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efefaee229
ℹ️ 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".
efefaee to
0ac8019
Compare
|
Rebased onto main
All threads resolved. Waiting on Codex to review this head before merging. |
|
@codex please review this PR |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 38 minutes. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ac8019d28
ℹ️ 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".
|
Round update, head P1 — discriminator for an updated code-first table. The serious one, and I had written a comment there asserting the opposite: "This table was created by the DDL a few lines above… not a guess." One grep disproves it — P2 — skip introspection for non-type filters. P2 — batch the boot introspection. The callback now collects rows and registers in a second pass with one Verification: All threads resolved. Re-triggering Codex on this head; merging once it reports clean, per the founder's grant. |
|
@codex please review this PR |
An exact spelling comparison called a folded registry absent, which let the derived legacy names be declared usable and the reload build empty tables beside the populated migrated ones.
A group whose schema change was skipped had its new fields persisted anyway, so the registry described columns the table does not have and the next boot built a runtime schema that failed every read and write for it.
Prefix discovery is the only route to an orphan whose registry row is what went missing, and after the storage migration a generated table carries the migrated prefix rather than the legacy one.
The count over a component filter repeated the registry lookup and the catalog introspection the list had already done for the same request.
The orphan sweep reaches field-group tables by walking the catalog, so a migrated localized group's companion entered it and was probed as an instance table for an id column it does not have, failing every entity delete.
A catalog read that failed sent every field group to the legacy discriminator, which on migrated storage names a column none of those tables have, so all field-group reads and writes failed until the process restarted.
2862aca to
2ccdc53
Compare
|
Rebased onto Main's only new commit (#462) touches two Verification on this head: The three webhook outbox failures I reported last round as pre-existing on |
|
@codex please review this PR |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 40 minutes. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ccdc53568
ℹ️ 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 migrated database holds both discriminator spellings at once, since a group created after the migration carries the legacy column beside migrated siblings, so no answer derived from the database as a whole is right for every table. The batch probe now falls back to asking per table and leaves out what it cannot read, and the boot pass skips those rather than naming a column that was never verified.
|
@codex please review this PR |
|
Codex Review: Didn't find any major issues. 🚀 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". |
1 participant
What
The expand step of expand → migrate → contract for task 011. Every reader that addresses field-group storage now resolves the name from the database catalog instead of a constant, so one build reads both generations correctly.
This changes no storage. It adds no column, moves no data, and does not touch the merged migration engine. A database that has not migrated behaves exactly as before.
Why now
The three-dialect matrix (#448) found on its first real run that the migration renames things no runtime code can address.
MIGRATION_TARGET.columnTypeappeared nowhere outside the migration folder, andFieldGroupRegistryService.registryTableNamewas a constant — so after a successfulup, zero runtime schemas register and every field group reads as absent. B1 and B2 therefore could not ship independently. Expand is what separates them.The rule
Applied identically to the registry table and to each data table's discriminator. It is the order the migration itself uses, and the only one correct in both directions:
uprenames the registry last,downrenames it first, so "legacy if present" always names the live object and never adopts one this migration did not move.🔴 Resolved from the catalog, never from the marker. The marker is a recorded claim; the catalog is an observable fact. A database restored from backup can carry a marker that disagrees with its own storage — which is exactly the case a reader has to survive.
guard.ts'sresolveStorageVerdictis not the seam: it combines the marker with the probe and throws on every pair it cannot explain, so wiring it into the read path would turn a restored database from "reads correctly" into "refuses to boot".Two design points worth reviewing closely
1. The Drizzle property key stays put; only the physical name moves. The adapter addresses columns by property key everywhere —
db.select().from(tableObj)returns rows keyed by property,buildDrizzleWhereresolvesgetColumns(table)[cond.column],mapDataToColumnNamesfalls back to the key it was given. Keeping the key equal toSTORAGE_FORMAT.columns.typetherefore leaves every consumer of a component row unchanged, and keeps them correct through B2 when that constant flips.The registry table gets no such luck:
SchemaRegistry.registerStaticSchemaskeys a table bygetTableName(value)— the physical name — so there the handle is the name. Both the Drizzle object and the string that addresses it had to move together, which is why the dialect schemas became factories and both spellings are registered (for the schema registry only, never for a schema push).2. The discriminator is resolved per table, not once per database. Three independent reasons, none hypothetical: the registry renames last, so mid-run and post-crash the two generations coexist;
retargetNamereturnsnullfor an author-named table, so its column moves while its table does not; and the DDL keeps writing the legacy spelling for a whole release, so a group created after a migration carries the legacy column while its siblings carry the migrated one.typeColumnis required, not defaultedA default would let a call site that never learned to resolve compile and then silently project a column that is not there — the exact failure this PR exists to remove. Making it required turned the type checker into the completeness proof: it named all seven call sites. Each one supplies either the resolved value, or
STORAGE_FORMAT.columns.typewhere the code just wrote that DDL itself (the same constant read twice, not a guess).pushschema-pipeline.tsdeliberately uses the constant — it builds the desired schema handed to drizzle-kit, so resolving there would make the desired shape follow the live shape and the diff always empty.Tests
reads content through the typed CRUD on either generation— the assertion this slice exists for, and the one the suite could not make before. The same code reads content through the typed ORM before and after the migration, choosing the discriminator from the catalog rather than being told which generation it is in. Proven load-bearing: making the resolution always answer the legacy spelling failed exactly and only that test, on both Postgres and SQLite, with the test count unchanged.registerComponentSchemasgets a test that fails when the registry resolution is reverted to the constant.🔴 A gap this does NOT close, and it blocks the entry point
getCoreSchema(dialect)includes the field-group registry, andreconcileCoreintrospectsCORE_TABLE_NAMES(which listsdynamic_components) and diffs. On a migrated database running this release's code, that diff sees the legacy registry missing and emits a CREATE — an empty duplicate.ensureSystemTablesdoes the same viaCREATE TABLE IF NOT EXISTS. On the next boot, legacy-first resolution would then pick the empty one.It is not reachable today, because nothing migrates storage without the entry point, which does not exist. It is filed as the next slice and must land before the entry point: without it, the rollback safety that justified expand→migrate→contract does not exist. Keeping it out of this PR is deliberate —
reconcileCorecan drop tables and earns its own review rather than riding along.Verification
pnpm build✅ ·pnpm lintexit 0 ✅pnpm check-types --force: 8 errors, byte-identical tomain's own (i18n/companion + collection-bulk test files, from fix(nextly): resolve companion readiness once and stop the read path aborting transactions #429, none touched here). Verified by running it on a cleanorigin/mainworktree. This branch introduces none.Summary by CodeRabbit
Bug Fixes
Tests