fix(nextly): resolve companion readiness once and stop the read path aborting transactions - #429
Merged
Merged
Conversation
Provisioning skipped every entity that was not currently localized, so setting `localized: false` in configuration abandoned the companion holding all the content and fell back to the main table's retained columns. Those hold whatever they held before the entity was localized, because every write since had gone to the companion alone, so the edits were still on disk and no longer visible. The restore runs after the schema push, which is what puts the columns back, and copies the default locale's companion values onto main. It drops and archives nothing: db:sync persists registry metadata before its destructive prompt, so a drop would run even for an operator who then declined the change, and with the companion still standing there is nothing an archive would preserve. Leaving the companion in place would otherwise create the mirror trap, so the transition record gains a `restored` state. Re-enabling localization then overwrites those default-locale rows from main instead of trusting them, since main was authoritative for the whole period localization was off. The store is now resolved without requiring a configured default locale. An app that removes its `localization` block entirely is exactly the case that owes a restore, and demanding a locale first hid those entities; the recorded source locale stands in when configuration no longer names one.
…r atomically Two defects that both come of trusting something other than what is actually there at the moment the statement runs. The seed read `status` from the main table whenever the entity had Draft/Published in its desired configuration. One edit turning on localization and Draft/Published together reaches the pre-apply copy with no `status` column yet, so the seed failed after the companion had been created — and every later run found that companion and resumed into the same statement, so that combination could never apply. The status copy is now gated on the physical column. Nothing is lost by skipping it: main's column and the companion's `_status` are both created NOT NULL DEFAULT 'draft', so rows gaining Draft/Published in this edit reach the same state either way. Recording the first transition went through a check-then-act write, so two processes provisioning the same entity both read `untracked` and both wrote. The one that lost the companion CREATE could still record the language, leaving the marker naming a locale the seed never used — which defeats the only fact the record exists to keep. The first write is now a conditional insert resolved by the database, and the caller re-reads to see what landed: agreeing on the locale is not a loss worth reporting, disagreeing is fatal.
…h the pre-apply snapshot Three findings, all about a decision outliving the state it was made from. A disable migration took its prior main-table shape from live introspection, so a file generated against a development database whose unattended transition retained its columns omitted their ADD COLUMN. Replayed where the enable migration had dropped them, the restore addressed columns that were not there. The artefact is now derived from history alone and the local shape produces a second plan, returned only when it differs, so a caller holding one plan cannot pick the wrong one. The file is what is saved; the local plan is what is run. The pre-apply transition can relax a retained column, and on SQLite — which cannot change nullability — drops one instead. The reload had already cached the live snapshot the pipeline reuses, so the apply re-emitted a DROP for a column that was already gone. The cache is dropped when the pass changed anything, which costs one introspection and only in a cycle where a transition happened. A comment named a pull request. Comments describe code.
… every write Three consumers were asking the database the same question — provisioning, the localized write guard, and the read-back that builds a response — and each asked it its own way, per write. An entry whose dynamic zone holds K localized field-group types paid 1 + K round trips before its transaction and K more inside it. Local SQLite hides that; managed PostgreSQL does not, and the cost grows with the complexity of the content. Readiness is now three states rather than a boolean, because "no companion" splits into a legitimate main-table fallback and a state that must be refused, and a boolean would leave the introspection that tells them apart on the write path. Only `ready` is remembered: it is the healthy steady state where the whole per-write cost lives, and it is reached by creating a table, which no ordinary operation undoes. The abnormal states re-resolve every time, so an entity mid-transition keeps the freshness it has today rather than trading it for speed it does not need. That also closes the read-path abort. companion-join decided the companion existed by running the join and catching the failure, at five sites — free on SQLite and MySQL, and on PostgreSQL a statement that errors marks the whole transaction aborted, so the next one dies with `current transaction is aborted` and takes the blame. Several of those reads run inside the caller's write transaction, so the check was causing the failure it was written to tolerate. Readiness is now an argument, required on every reader so a new caller cannot omit it, and nothing in that module catches: not ready means no query, ready means every failure propagates. The `strict` flag existed only to opt out of the swallow and goes with it. The presence map #382 threaded from a pre-transaction pass through three services collapses into the same lookup, since resolving readiness before the transaction is what the in-transaction path now reads. 086's reproduction, removed from #382 because it could not pass, is restored. It runs against a suite that can finally see the failure: the merged aborted-transaction guard fails any run that leaves a transaction poisoned, so a reintroduction trips on its own.
…t refusal The refusal an editor hits when translations have nowhere to go named `nextly db:sync` in every environment. In production that is wrong advice: boot deliberately refuses to run DDL there, so the development remedy cannot help, and naming it costs the operator the time to try it before they start looking for the real answer. The message now branches, and the three copies of it that had begun to drift are one function. `nextly migrate` becomes the remedy it names. It provisions companions after the migrations have run, which is the only supervised path available to a deployment that may not alter its own schema at boot, and the only one that can repair an install which transitioned before transitions were recorded. Those have a companion and no marker, so nothing can tell whether their content was ever copied across — the one fact that cannot be re-derived is the language, and running the repair supplies it from the configured default. The copy is guarded on rows with no default-locale companion row, so an install that does not need it gets a scan and nothing else, and translations written since are never overwritten. Absence is read as a debt ONLY under supervision. An entity localized from birth is untracked too and owes nothing, so an unattended pass must keep treating the two the same.
Collaborator
Author
|
@codex please review this PR |
|
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 (30)
📝 WalkthroughWalkthroughThis change adds readiness-aware localized companion access, restoration when localization is disabled, opt-in repair for legacy companions, atomic transition tracking, local migration SQL, and broad integration coverage for provisioning, recovery, and failure handling. ChangesLocalized companion lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
@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: |
![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: cbb8263e4c
ℹ️ 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".
…nion read failures Three review findings. Two are one missing invariant. A seed debt that continues an existing `enabling` record can go straight to the copy — that record is what it continues. The other two ways a debt arises are NEW transitions and were not recording one, which broke both of them. The supervised repair copied and then called settle, and settle refuses an entity with no record: nothing established that a copy ran or in which language. So `nextly migrate` reported a provisioning failure after modifying the database, left the marker untracked, and failed identically on every retry. A companion that outlived a disable reused the locale the restore recorded. Main has been authoritative since that disable and carries no language of its own, so enabling now declares its content to be in TODAY's default, exactly as a first enable does. Reusing the restore's locale labels the rows with a code reads no longer look for — one that may not even be configured any more — so every edit made while localization was off disappears the moment it comes back on. Both now claim a transition at the current default before any copy, which is also this module's own ordering rule: the record goes in before the statements, because MySQL commits DDL implicitly. Separately, a Single read no longer hands the driver's own words to the response. Companion reads used to swallow a failure, so only the access-rule path could throw and only that path wrapped the error; now every failure propagates, and the result builder was putting the failed query — companion table and column names included — straight onto the wire.
Collaborator
Author
|
@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: 575878151d
ℹ️ 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".
…res atomic Four review findings, each a place where something that looked settled was not. Readiness was remembered by table name alone, on a process-wide set. A table name does not identify a table: one process can hold two adapters, and the first database's verdict then vouched for a companion the second had never provisioned, so its reads and writes addressed a missing table instead of taking the pre-migration fallback. Keyed on the adapter now, through a WeakMap, so identity does the scoping and a discarded adapter takes its verdicts with it. The clearest evidence it was mis-keyed is what the fix deletes: the package test setup no longer has to wipe the cache after every test. Re-enabling a `restored` entity took an unconditional write. Two processes doing that during a default-locale rollout both read `restored` and both proceeded under their own locale, labelling one main table's content as two languages while the marker recorded whichever landed last. It is a conditional move now, resolved in the database, with the loser re-reading and refusing when the winner chose differently. `MetaService` gains the compare-and-set that pairs with the insert-if-absent added earlier: one settles a race to create a key, the other a race to move one. A multi-column restore ran one statement per column, so a failure part-way left main carrying a mixture of restored and pre-localization values with nothing recording that a restore was attempted — after which the app served that mixture, accepted edits on it, and the next pass overwrote them from the stale companion. One statement covers every column, so it either happened or it did not. `nextly migrate --step` provisioned companions from the final config while later migrations were still pending, creating a companion that a pending migration was about to create for itself. Provisioning now waits until nothing is pending, and says so.
mobeenabdullah
force-pushed
the
feat/i18n-companion-readiness
branch
from
July 31, 2026 12:21
d9cf39b to
25f583e
Compare
Collaborator
Author
|
@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: 25f583e17b
ℹ️ 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".
… on re-enable Four review findings. The supervised repair read the absence of a transition record as a debt. It is not: an entity with no record is either an install that enabled localization before Nextly recorded transitions, or one localized since birth that owes nothing — and nothing on disk tells them apart, since the push leaves the translatable columns on main in both cases. Inferring it manufactured a default-locale translation for every entry, including ones deliberately authored in another language only, and recorded a transition that never happened. It is now `nextly migrate --repair-localization`, which is the operator supplying the one fact that cannot be recovered: that their main tables hold content in the configured default locale. That is the same conclusion the rest of this mechanism rests on, applied to the one place I had reached for a guess. The transition is also no longer recorded while resolving the debt, only once the plan is known to describe real work, so a repair that turns out to be owed nothing leaves no trace. Re-enabling refreshed the localized values and left `_status` alone. Publishing state moves while localization is off too, and the companion row that survives the disable keeps whatever status it had when it was last the authority — so a page published in the meantime stayed hidden from locale-aware published reads, and the guarded insert could not correct it, because the row it would fix already exists. Collection reads now normalize a companion failure the way the Single reads do. These reads used to swallow one, so nothing here had to shape their errors; now every failure propagates and `listEntries` was putting the failed query — with companion table and column names in it — into its own result message. A failed restore during an HMR reload only warned, and the reload went on to publish the non-localized configuration. The app then read the stale main values and accepted edits on them, and a later successful retry copied the companion's older values over the top. It now stops, exactly as the pre-apply preservation path does, and says the content is intact where it is.
Collaborator
Author
|
@codex please review this PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nextly/src/domains/collections/services/collection-mutation-service.ts (1)
1246-1256: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winResolve readiness on pooled
readCompanionSlugsAllLocalescalls.
cachedCompanionReadiness()returnsundefinedwhen the adapter has not seen areadyverdict, and onlyreadyletspopulateCompanionFieldsAllLocales()join the companion. On the post-commitpublishAllLocalespath, the first cold call leavesrow.slugunset, returns[], and completes without the warning that other read failures produce. Resolve readiness from the pooled connection before calling this helper, or pass the resolved readiness in from the transaction-bound path.🤖 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/collections/services/collection-mutation-service.ts` around lines 1246 - 1256, The cachedCompanionReadiness call in populateCompanionFieldsAllLocales returns undefined when the adapter has not seen a ready verdict, preventing the companion join and leaving row.slug unset without proper warnings. Resolve the readiness from the pooled connection before passing it to populateCompanionFieldsAllLocales, or obtain the resolved readiness from the transaction-bound context where it is already available, ensuring the readiness parameter is never undefined so the companion table join succeeds.
🧹 Nitpick comments (7)
packages/nextly/src/domains/dynamic-collections/services/dynamic-collection-service.ts (1)
719-735: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated transition-assembly logic in
generateCollectionUpdate. Both branches repeat the same destructure,archiveSQLcomputation andassembleclosure. The shared root cause is that the assembly was copied into each branch instead of extracted, so the archive-flag question and every future ordering fix must be answered twice.
packages/nextly/src/domains/dynamic-collections/services/dynamic-collection-service.ts#L719-L735: replace the inlinearchiveSQLandassemblewith one private helper that returns bothmigrationSQLandlocalMigrationSQL, and resolve the archive flag for the local plan there.packages/nextly/src/domains/dynamic-collections/services/dynamic-collection-service.ts#L771-L797: call the same helper withmainSQL,companionSQL,localCompanionSQLandneedsArchiveinstead of repeating the block.🤖 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/dynamic-collections/services/dynamic-collection-service.ts` around lines 719 - 735, The transition assembly is duplicated in generateCollectionUpdate. In packages/nextly/src/domains/dynamic-collections/services/dynamic-collection-service.ts lines 719-735, extract the archiveSQL calculation and assemble closure into one private helper returning migrationSQL and localMigrationSQL, resolving the archive flag for the local plan within that helper. In lines 771-797, replace the repeated assembly block with calls to the same helper using mainSQL, companionSQL, localCompanionSQL, and needsArchive.packages/nextly/src/domains/i18n/__tests__/localized-write-without-companion.integration.test.ts (1)
146-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
dropCompanionhere.
enterWindowrepeats the drop-then-forget sequence thatdropCompanionnow owns. Calling the helper keeps one place that decides what "companion gone" means for this file.♻️ Proposed refactor
const handle = await boot(true); - await handle.adapter.executeQuery( - "DROP TABLE IF EXISTS dc_i18nwin_posts_locales" - ); - forgetCompanionReadiness(handle.adapter, "dc_i18nwin_posts_locales"); + await dropCompanion(handle, "dc_i18nwin_posts_locales", "sqlite"); return handle;🤖 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/i18n/__tests__/localized-write-without-companion.integration.test.ts` around lines 146 - 155, Update enterWindow to call the existing dropCompanion helper for dc_i18nwin_posts_locales instead of directly executing DROP TABLE and calling forgetCompanionReadiness. Preserve the existing teardown and return behavior while centralizing companion-removal semantics in dropCompanion.packages/nextly/src/domains/field-groups/services/field-group-mutation-service.ts (1)
132-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the
txobject parameter with an explicit discriminator.
splitLocalizedComponentnow usestxonly to decide betweencachedCompanionReadinessandresolveCompanionReadiness. It never readstx.adapter. The three in-transaction call sites still build a wrapper withthis.txWriteAdapter(tx)(lines 682, 893, 1220) whose value is discarded.Narrow the parameter to what the function actually needs. A boolean, or better an explicit
readinessargument, makes the contract visible and removes three unused allocations.♻️ Proposed refactor
private async splitLocalizedComponent( meta: DynamicFieldGroupRecord, data: Record<string, unknown>, locale: string | undefined, - tx?: { - adapter: { - dialect: SupportedDialect; - executeQuery<T = unknown>( - sql: string, - params?: unknown[] - ): Promise<T[]>; - }; - } + // True when this runs on a caller's transaction connection. Readiness is then READ from the + // cache, never resolved: resolving issues a query, and a query against a missing relation + // aborts the whole transaction on PostgreSQL. + insideTransaction = false ): Promise<{- const readiness = tx + const readiness = insideTransaction ? cachedCompanionReadiness(this.adapter, schema.companionTableName) : await resolveCompanionReadiness(this.adapter, {Then update the three call sites, for example line 682:
- { adapter: this.txWriteAdapter(tx) } + trueAlso applies to: 192-198
🤖 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/field-groups/services/field-group-mutation-service.ts` around lines 132 - 144, Update splitLocalizedComponent to replace its tx object parameter with an explicit readiness discriminator, preferably a readiness argument that selects cachedCompanionReadiness for transaction paths and resolveCompanionReadiness otherwise. Remove the unused adapter wrapper from the three in-transaction call sites using this.txWriteAdapter(tx), and pass the corresponding discriminator directly while preserving pooled-path behavior.packages/nextly/src/domains/collections/services/collection-query-service.ts (1)
216-217: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winResolve companion readiness once per read and pass it to the three overlays.
populateLocalized,populateLocalizedAll, andpopulateTranslationMetaeach callresolveCompanionSchemaReadinessindependently. A list read withtranslationStatus: truetherefore resolves the same verdict up to three times.Two consequences:
resolveCompanionReadinesscaches only thereadyverdict. For apre-migrationorbrokencompanion, every call re-runscompanionTableExistsandmainTableHasColumns, so an unmigrated localized collection pays extra introspection queries on every request.- The three calls can observe different verdicts within one response if provisioning completes mid-request. The response would then carry unresolved localized fields together with a populated
_translationsmap.Resolve the verdict once beside the
companionload inlistEntriesandgetEntry, then pass it into each helper.Also applies to: 249-249, 350-350
🤖 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/collections/services/collection-query-service.ts` around lines 216 - 217, Move the resolveCompanionSchemaReadiness call to execute once per read operation in the listEntries and getEntry methods, storing the result alongside the companion load. Then refactor the three helper functions populateLocalized, populateLocalizedAll, and populateTranslationMeta to accept the pre-resolved readiness as a parameter instead of calling resolveCompanionSchemaReadiness independently within each function. Update all calls to these three helpers to pass the resolved readiness value.packages/nextly/src/domains/collections/services/collection-mutation-service.ts (1)
1003-1008: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
resolveCompanionSchemaReadinessinstead of re-deriving the main table name.
resolveCompanionSchemaReadinessinpackages/nextly/src/domains/i18n/runtime/companion-readiness.tsalready strips the_localessuffix and mapslocalizedFieldsto columns. This block repeats both steps. A single helper keeps the two derivations from drifting.♻️ Proposed refactor
- const mainTableName = companion.companionTableName.replace(/_locales$/, ""); - const readiness = await resolveCompanionReadiness(this.adapter, { - companionTableName: companion.companionTableName, - mainTableName, - localizedColumns: companion.localizedFields.map(f => f.column), - }); + const readiness = await resolveCompanionSchemaReadiness( + this.adapter, + companion + );🤖 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/collections/services/collection-mutation-service.ts` around lines 1003 - 1008, Replace the manual derivation of mainTableName and localizedColumns columns with a call to resolveCompanionSchemaReadiness instead of resolveCompanionReadiness. The resolveCompanionSchemaReadiness helper in packages/nextly/src/domains/i18n/runtime/companion-readiness.ts already handles both the _locales suffix stripping and the localizedFields to columns mapping internally, so pass the companion object directly to it rather than pre-deriving these values separately to keep the transformation logic centralized and prevent drift.packages/nextly/src/domains/meta/services/meta-service.ts (1)
160-177: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
compareAndSetcompares serialized JSON, so every caller must reproduce the exact byte form.The
WHEREclause matchesJSON.stringify(expected)against the stored string. The comparison therefore depends on property order and on the driver's string collation.beginI18nTransitionrebuilds the expected marker by hand from a read value, so a future field added toStoredMarker, or a writer that builds the literal in a different order, makes the update match zero rows without any error.Serialize both sides through one canonical helper (stable key order), and document the constraint on the method.
🤖 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/meta/services/meta-service.ts` around lines 160 - 177, Update compareAndSet and its callers to use a shared canonical JSON serialization helper with stable object-key ordering for both expected and next values, ensuring comparisons are independent of property order; replace beginI18nTransition’s hand-built marker reconstruction with this helper, and document the canonical serialization requirement on compareAndSet.packages/nextly/src/domains/i18n/migration/transition-state.ts (1)
396-415: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
recordI18nRestorewrites without a claim, unlike every other transition write.The function reads the marker at Line 399 and writes it at Line 414 with
store.set. The two steps are not atomic. A concurrentbeginI18nTransitionthat moves the marker toenablingbetween them is overwritten withrestored, and the enable then loses its record.The store already exposes
compareAndSet. Use it here so the restore write is a claim as well, and re-read on a lost claim.🤖 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/i18n/migration/transition-state.ts` around lines 396 - 415, Update recordI18nRestore to write the restored marker through store.compareAndSet using the marker state read by readI18nTransitionState as the expected value. If the compare-and-set loses the claim, re-read the transition state and retry the validation/write flow so a concurrent beginI18nTransition is not overwritten.
🤖 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/cli/commands/__tests__/db-sync-localized-companion.integration.test.ts`:
- Around line 742-747: Update the comment in the test case around
ensureLocalizedCompanions to refer to the documented operator flag
--repair-localization instead of the internal --supervised name, while
preserving the explanation that this scenario corresponds to repairUntracked:
true.
In
`@packages/nextly/src/domains/dynamic-collections/services/dynamic-collection-service.ts`:
- Around line 414-421: Filter args.oldFields through
resolveLocalizedFieldNames(args.oldFields, args.wasLocalized) before passing
them to localizedColumnsOnMain in the buildCompanionTransitionPlans setup. Keep
the existing column-name mapping and ensure existingMainColumns uses only
resolved localized field names, matching the disable-path translatable-column
calculation.
In `@packages/nextly/src/domains/i18n/companion-join.integration.test.ts`:
- Around line 93-98: Remove the work-item reference from the comment above the
“does not touch the database at all when the companion is not ready” test in
packages/nextly/src/domains/i18n/companion-join.integration.test.ts:93-98,
preserving the technical explanation about catch-based existence checks aborting
PostgreSQL transactions. Also remove the PR reference from the
aborted-transaction guard comment in
packages/nextly/src/domains/i18n/__tests__/localized-write-without-companion.integration.test.ts:566-571,
keeping only its code rationale.
In `@packages/nextly/src/domains/i18n/migration/generate-down.ts`:
- Around line 42-44: The documentation headers for the migration builders
incorrectly describe one correlated UPDATE per column. Update the headers in
packages/nextly/src/domains/i18n/migration/generate-down.ts lines 42-44 and
packages/nextly/src/domains/i18n/migration/generate-up.ts lines 180-182 to state
that a single correlated UPDATE covers every column in columnNames, and revise
the closing note in generate-up.ts lines 202-203 to remove the per-column
wording.
In `@packages/nextly/src/domains/i18n/migration/generate-up.ts`:
- Around line 220-222: Update the refresh assignment in the
options.refreshStatus branch to apply a 'draft' fallback when main.status is
NULL before copying it into _status. Preserve the existing copy behavior for
non-NULL statuses and ensure the resulting value satisfies the companion _status
NOT NULL constraint.
In `@packages/nextly/src/domains/i18n/migration/transition-state.ts`:
- Around line 290-341: Update confirmClaim to require both claimed.status ===
"enabling" and claimed.sourceLocale === args.sourceLocale before returning.
Treat any observed marker with a different status or locale as a failed claim
and throw the existing NextlyError.internal with the relevant recorded values,
ensuring callers only proceed when an enabling record was actually claimed.
In `@packages/nextly/src/domains/i18n/runtime/restore-companion.ts`:
- Line 102: Update restoreLocale and the restored-recording flow in the restore
handler so a configured default locale without a companion row does not produce
a terminal restored record. Prefer recorded.sourceLocale when the default-locale
companion is absent, or only record restored after confirming an UPDATE affected
a row; preserve existing behavior when a matching companion exists.
- Around line 112-118: Update the companion-absence branch in the restore flow
around localizedColumnsOnBothTables to probe the table through the adapter
before calling forgetI18nTransition. Only forget the transition and return false
when the adapter confirms the companion table is absent; preserve the restore
record and continue the existing flow when the adapter finds the table,
including tables outside public.
In `@packages/nextly/src/domains/meta/services/meta-service.ts`:
- Around line 129-142: Update insertIfAbsent to throw an explicit
unsupported-operation error when neither onConflictDoNothing nor
onDuplicateKeyUpdate is available, and remove the final plain await insert
fallback. Preserve the existing conflict-clause branches and their return
behavior.
In `@packages/nextly/src/domains/singles/services/single-mutation-service.ts`:
- Around line 870-878: Use companionPhysicallyExists to guard the later
companion-status read and upsert paths, alongside the existing companion/status
conditions. Ensure _status is neither queried nor written unless
resolveCompanionReadiness reports "ready", while preserving the untracked
companion default-locale fallback.
- Around line 917-920: Update the companionReadiness check in the
single-mutation service to compare against the contract’s "unavailable" state
instead of "broken", ensuring refuse() runs before the transaction when the main
table cannot store localized fallback columns.
In `@packages/nextly/src/domains/singles/services/single-query-service.ts`:
- Around line 773-787: Update the comments at
packages/nextly/src/domains/singles/services/single-query-service.ts#L773-L787
and `#L1800-L1812` to describe present-tense normalized propagation of
companion-read and translation-overview failures, including the rationale.
Update the ordinary-read comment at
packages/nextly/src/domains/singles/services/single-query-service.ts#L1588 to
reflect that companion-read failures propagate. Update
packages/nextly/src/domains/singles/services/single-mutation-service.ts#L256-L260
to explain cached readiness for transactional reads, and `#L1235-L1238` to explain
why genuine webhook pre-image read failures abort the write; remove historical
“used to”/“now” remediation wording.
In `@packages/nextly/src/init/reload-config.ts`:
- Around line 1726-1747: Update the no-DDL provisioning path that calls
ensureLocalizedCompanionsForReload to retain its result instead of discarding
it. When the returned restoreFailed list is non-empty, log the same
localization-stays-on message, invoke the path’s existing reload-abort mechanism
(such as abandonReload), and return before syncCodeFirstMetadataOnly or any
registry publication proceeds.
---
Outside diff comments:
In
`@packages/nextly/src/domains/collections/services/collection-mutation-service.ts`:
- Around line 1246-1256: The cachedCompanionReadiness call in
populateCompanionFieldsAllLocales returns undefined when the adapter has not
seen a ready verdict, preventing the companion join and leaving row.slug unset
without proper warnings. Resolve the readiness from the pooled connection before
passing it to populateCompanionFieldsAllLocales, or obtain the resolved
readiness from the transaction-bound context where it is already available,
ensuring the readiness parameter is never undefined so the companion table join
succeeds.
---
Nitpick comments:
In
`@packages/nextly/src/domains/collections/services/collection-mutation-service.ts`:
- Around line 1003-1008: Replace the manual derivation of mainTableName and
localizedColumns columns with a call to resolveCompanionSchemaReadiness instead
of resolveCompanionReadiness. The resolveCompanionSchemaReadiness helper in
packages/nextly/src/domains/i18n/runtime/companion-readiness.ts already handles
both the _locales suffix stripping and the localizedFields to columns mapping
internally, so pass the companion object directly to it rather than pre-deriving
these values separately to keep the transformation logic centralized and prevent
drift.
In
`@packages/nextly/src/domains/collections/services/collection-query-service.ts`:
- Around line 216-217: Move the resolveCompanionSchemaReadiness call to execute
once per read operation in the listEntries and getEntry methods, storing the
result alongside the companion load. Then refactor the three helper functions
populateLocalized, populateLocalizedAll, and populateTranslationMeta to accept
the pre-resolved readiness as a parameter instead of calling
resolveCompanionSchemaReadiness independently within each function. Update all
calls to these three helpers to pass the resolved readiness value.
In
`@packages/nextly/src/domains/dynamic-collections/services/dynamic-collection-service.ts`:
- Around line 719-735: The transition assembly is duplicated in
generateCollectionUpdate. In
packages/nextly/src/domains/dynamic-collections/services/dynamic-collection-service.ts
lines 719-735, extract the archiveSQL calculation and assemble closure into one
private helper returning migrationSQL and localMigrationSQL, resolving the
archive flag for the local plan within that helper. In lines 771-797, replace
the repeated assembly block with calls to the same helper using mainSQL,
companionSQL, localCompanionSQL, and needsArchive.
In
`@packages/nextly/src/domains/field-groups/services/field-group-mutation-service.ts`:
- Around line 132-144: Update splitLocalizedComponent to replace its tx object
parameter with an explicit readiness discriminator, preferably a readiness
argument that selects cachedCompanionReadiness for transaction paths and
resolveCompanionReadiness otherwise. Remove the unused adapter wrapper from the
three in-transaction call sites using this.txWriteAdapter(tx), and pass the
corresponding discriminator directly while preserving pooled-path behavior.
In
`@packages/nextly/src/domains/i18n/__tests__/localized-write-without-companion.integration.test.ts`:
- Around line 146-155: Update enterWindow to call the existing dropCompanion
helper for dc_i18nwin_posts_locales instead of directly executing DROP TABLE and
calling forgetCompanionReadiness. Preserve the existing teardown and return
behavior while centralizing companion-removal semantics in dropCompanion.
In `@packages/nextly/src/domains/i18n/migration/transition-state.ts`:
- Around line 396-415: Update recordI18nRestore to write the restored marker
through store.compareAndSet using the marker state read by
readI18nTransitionState as the expected value. If the compare-and-set loses the
claim, re-read the transition state and retry the validation/write flow so a
concurrent beginI18nTransition is not overwritten.
In `@packages/nextly/src/domains/meta/services/meta-service.ts`:
- Around line 160-177: Update compareAndSet and its callers to use a shared
canonical JSON serialization helper with stable object-key ordering for both
expected and next values, ensuring comparisons are independent of property
order; replace beginI18nTransition’s hand-built marker reconstruction with this
helper, and document the canonical serialization requirement on compareAndSet.
🪄 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.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 288cceb4-c909-408d-86cf-aefba2e6d939
⛔ Files ignored due to path filters (1)
.changeset/i18n-companion-readiness.mdis excluded by!.changeset/**
📒 Files selected for processing (37)
packages/nextly/src/cli/commands/__tests__/db-sync-localized-companion.integration.test.tspackages/nextly/src/cli/commands/dev-build.tspackages/nextly/src/cli/commands/migrate.tspackages/nextly/src/dispatcher/handlers/collection-dispatcher.tspackages/nextly/src/dispatcher/handlers/component-dispatcher.tspackages/nextly/src/dispatcher/handlers/single-dispatcher.tspackages/nextly/src/domains/collections/services/collection-metadata-service.tspackages/nextly/src/domains/collections/services/collection-mutation-service.tspackages/nextly/src/domains/collections/services/collection-query-service.tspackages/nextly/src/domains/dynamic-collections/services/dynamic-collection-service.tspackages/nextly/src/domains/field-groups/services/field-group-data-service.tspackages/nextly/src/domains/field-groups/services/field-group-mutation-service.tspackages/nextly/src/domains/field-groups/services/field-group-query-service.tspackages/nextly/src/domains/i18n/__tests__/localized-write-without-companion.integration.test.tspackages/nextly/src/domains/i18n/companion-join.integration.test.tspackages/nextly/src/domains/i18n/companion-join.test.tspackages/nextly/src/domains/i18n/companion-join.tspackages/nextly/src/domains/i18n/migration/generate-down.test.tspackages/nextly/src/domains/i18n/migration/generate-down.tspackages/nextly/src/domains/i18n/migration/generate-up.tspackages/nextly/src/domains/i18n/migration/reconcile-companion.tspackages/nextly/src/domains/i18n/migration/teardown-entity-i18n.tspackages/nextly/src/domains/i18n/migration/transition-plans.test.tspackages/nextly/src/domains/i18n/migration/transition-recorder.tspackages/nextly/src/domains/i18n/migration/transition-state.test.tspackages/nextly/src/domains/i18n/migration/transition-state.tspackages/nextly/src/domains/i18n/migration/types.tspackages/nextly/src/domains/i18n/runtime/companion-io.tspackages/nextly/src/domains/i18n/runtime/companion-readiness.test.tspackages/nextly/src/domains/i18n/runtime/companion-readiness.tspackages/nextly/src/domains/i18n/runtime/restore-companion.tspackages/nextly/src/domains/i18n/translation-status.integration.test.tspackages/nextly/src/domains/meta/services/meta-service.tspackages/nextly/src/domains/singles/__tests__/localized-single-without-companion.integration.test.tspackages/nextly/src/domains/singles/services/single-mutation-service.tspackages/nextly/src/domains/singles/services/single-query-service.tspackages/nextly/src/init/reload-config.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f9d99662d
ℹ️ 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".
The restore executed generated statement strings through `adapter.executeQuery`, which is a raw-SQL product data-access path and against the repository's Drizzle-only rule. It now goes through the query builder. Both table objects already have runtime builders — `generateRuntimeSchema` for main, `buildCompanionRuntimeTable` for the companion — so identifiers come from the generated columns rather than hand-quoting, and the locale is bound rather than embedded. The correlated subquery uses Drizzle's `sql` template with those column references, which is how a correlated copy is expressed in the builder. The pairing is the part worth knowing: the main table object is keyed by FIELD name while the companion is keyed by physical COLUMN name, so `subTitle` and `sub_title` are one value under two keys. They are paired through the same descriptor the columns were created from rather than by re-deriving the conversion. `buildDefaultLocaleRestoreStatements` stays, and stays tested: the disable MIGRATION still emits text, because a migration file has to carry SQL. What goes away is a runtime path executing that text.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53168a8553
ℹ️ 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".
…nditional marker writes A review pass over the whole branch. The three that could lose or strand content: The restore preferred the configured default locale unconditionally. Its copy is guarded on a matching companion row, so an entity only ever authored under the locale the transition recorded matched nothing, copied nothing — and the record still marked the transition finished. `restored` is terminal, so no later pass retried and the content stayed in a companion nothing reads. The locale is now chosen by which one the companion actually holds. A field turned `localized: false` while its entity stays localized keeps its companion column, because reconciliation is additive, while writes correctly go to the restored main column. A later entity-level disable then copied that abandoned value back over the current one. The per-field flags now decide which columns the companion still owns, and only fall back to the physical intersection when none of them claim anything. `settleI18nTransition` and `recordI18nRestore` wrote unconditionally. Opposite transitions can interleave — a disable can restore and record while a re-enable is still copying — and an unconditional write buries the other's state, after which the next enable trusts a companion that is stale. Both are conditional on the state they actually operated on now, and losing is not an error: whatever moved the entity owns it. Also: `confirmClaim` compared only the locale, so a claim lost for any non-concurrency reason passed as won and left the copy with nothing to settle; it now requires `enabling` too. A NULL main `status` violated the companion's NOT NULL `_status` on refresh, permanently, since the transition stayed unsettled and every pass replayed it. The companion upsert on the singles write path was not gated on the companion existing, so a payload carrying a status rolled back the very fallback write it was meant to allow. `insertIfAbsent` fell back to a plain insert when neither conflict clause was found, silently turning a claim into an unconditional write. The no-DDL reload path discarded `restoreFailed`, so a disable that produced no schema diff published the non-localized metadata anyway. Companion absence is confirmed through the write path's own probe before the record carrying the source locale is deleted. The disable planner's `existingMainColumns` is filtered to fields that were actually translatable. And two builder headers still described one statement per column after both became one statement covering every column.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d63756a317
ℹ️ 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".
…verdict Five findings, four of them the same shape from different directions: a verdict that is only ever READ inside a transaction has to be RESOLVED before one opens, and a verdict that outlives the schema it describes is worse than none. Deleting a localized entry on a fresh worker resolved nothing beforehand, so the in-transaction snapshot that builds the durable delete event read an unresolved verdict, treated it as unusable, and silently omitted every localized field. Readiness is warmed on the pool before that transaction opens. The same for field groups: the pre-transaction pass resolved only the component types the payload happened to write, while a version or webhook snapshot reads every component the entity holds. A type left out of the payload had no verdict inside the transaction and its localized values went missing from the durable record. Every PERMITTED type is now resolved. Resolving is not refusing — the refusal still walks only what the payload writes, because a permitted type whose companion is missing must not fail a save that never mentions it. A positive verdict was trusted for the lifetime of the adapter. Companions are dropped by disable migrations, and `nextly migrate` runs in a process that cannot reach into a live server's memory, so an old worker in a rolling deployment kept querying a table the database no longer had. There is no invalidation channel between those processes, so the staleness is bounded by time: thirty seconds, which costs one plan-only SELECT per entity per window on the paths that resolve — against one per write before any of this. The re-enable refresh still executed a generated statement string. It goes through the query builder now, like the restore, and both directions live in one module so the pair cannot drift and the fact they share — that the main table object is keyed by FIELD name while the companion is keyed by physical COLUMN name — is written down once.
Collaborator
Author
|
@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: 0d2bdbf08f
ℹ️ 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".
Collaborator
Author
|
@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: 9c55a937cf
ℹ️ 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".
…ly your own claim Restoring took each column's first non-null value across the candidate locales independently. A parent holding rows in BOTH, with one field untranslated in the preferred one, therefore took that field from the other language while its neighbours and its publishing status came from the preferred row: a mixed-language document written to the table that is authoritative from then on, with the record marking the restore terminally finished. The row is chosen once per parent by rank now, and every value comes from it. An entity with no transition record was skipped entirely, which stranded exactly the installs that predate those records. Whether a companion came from a legacy transition or from an entity localized since birth makes no difference when localization is being turned off: either way the content is in the companion, because every write since went there. What separates both from an entity that was never localized is that the never-localized one has no companion at all, so that is what decides now. The transition is established through the ordinary claim first, so two processes disabling at once cannot both copy, and one cheap probe answers before any introspection because almost every entity reaching here never had a companion. Taking over an unfinished transition is how a crashed run gets recovered, and nothing in the row distinguishes an abandoned claim from an active one — a wall-clock lease cannot either, since a copy over a large table outlasts any timeout while its holder is still running. So the takeover is made harmless rather than forbidden: a claim is settled only by the token that made it, and a holder displaced mid-copy can no longer declare the copy done on behalf of the claim that displaced it. Resuming an unfinished copy claims too, which both lets it record that it finished and serialises two runs resuming at once. The reload fixture's adapter resolved every raw statement, so the companion existence probe reported a companion for every entity and certified paths that cannot run against a database without one. It answers with a missing-table error now unless the fixture says otherwise.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 040ce91ad7
ℹ️ 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".
Collaborator
Author
|
@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: 040ce91ad7
ℹ️ 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".
Naming the locales a restore may copy from turned them into a filter, and the case where that bites hardest is the one where the names are weakest: removing the localization block leaves no configured default at all, so the only candidate is the locale recorded when localization was first switched on. An entry authored solely under a default adopted since then has no row there, keeps whatever main held before it was ever localized, and is marked restored anyway. The named locales rank the rows now instead of selecting them, with the locale itself breaking the tie, so every parent comes back from the row it actually has and only a parent with no row at all is left alone. A field the configuration declares shared is excluded in every branch, not only when something else is claimed. One made shared while its entity stayed localized keeps a companion column that the physical intersection accepts, so an edit that clears the last remaining flag in the same pass as the entity's would otherwise hand back every field and copy that abandoned translation over the value main has been authoritative for. A component's translations failing to read no longer takes its shared values with them. The overlay runs after the shared values are deserialized, and the component read above it replaces the whole field with null when anything throws, so a fault that costs the reader one translation was costing them the record. Contained at the overlay, and only off a transaction: on the caller's connection the failure has already aborted it, and there the error belongs to the query that caused it rather than to whatever runs next.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67257372e3
ℹ️ 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".
…done Settling only your own claim stops a displaced holder from closing someone else's, but it does not stop the work. A run that claims a re-enable, pauses, and resumes after another run has taken over, refreshed and settled still executes its refresh — overwriting from stale main-table values the translations the taker has already seeded and published. Checking ownership beforehand cannot close that, because the check and the update are separate round trips. So the claim travels into the statement: the refresh carries a WHERE the database evaluates alongside it, and a claim the row no longer names matches nothing. Recording a restore reported nothing when its conditional write lost. The copy has already written main by then, so a caller told nothing goes on to publish a non-localized configuration over a record that says otherwise, and the next enable trusts a companion that no longer describes the main table. The result is returned and the pass fails rather than reporting a restore that is half true. Deleting an entry warmed only the collection's own companion. The snapshot that becomes the durable delete event reads every embedded component through the transaction, where a verdict can only be consulted, so on a fresh worker those overlays were skipped and the last description of the row there will ever be went out without its translations.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b21e720a3c
ℹ️ 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".
Collaborator
Author
|
@codex please review this PR |
…settlement The migration that disables localization restored only the default locale, then archived the other languages and dropped the companion. A parent with no default-locale row was skipped by the guard and kept whatever main held before it was ever localized, while its actual content left with the table. The default is a preference now, as it already is at runtime: rows are ranked per parent and one is chosen, so every entry comes back from the row it has. The guard moves to the parent, which still leaves a row that never had a translation alone. Settling a seed reported nothing when its conditional write lost. A run whose claim was taken over mid-copy therefore reported success, no preservation failure was recorded, and the schema apply that follows was free to drop the main-table columns — whose values the new claimant may not have copied anywhere yet. The result is returned and provisioning abandons the entity instead. Settling twice with the same token still succeeds. The marker keeps its owner across the move, so a settlement recognises its own finished work rather than reading it as a takeover.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b21e720a3c
ℹ️ 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".
Found by sweeping for fixes that landed on one side of a symmetric pair, after three review rounds in a row caught exactly that mistake. The runtime restore learned to carry a row's publishing state back with its values; the disable MIGRATION never did. Publishing is per locale while an entity is localized, so a row published only under a non-default language holds that state on its companion row alone — and this path drops the companion immediately after restoring, so moving the content without the state it was published under puts a draft in front of the public, or takes live content down, with nothing left to correct it from. Gated on the entity having Draft/Published, because that is what puts `status` on main and `_status` on the companion; reading either without it fails the whole migration.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec7057b60b
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e859d662f
ℹ️ 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".
Collaborator
Author
|
@codex please review this PR |
|
Codex Review: Didn't find any major issues. Nice work! 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". |
…eadiness # Conflicts: # packages/nextly/src/domains/collections/services/collection-mutation-service.ts
A regression from the commit before this one. The status restore was gated on `spec.status`, which is the shape the collection is being saved AS, not what its tables currently carry. A save that disables localization and turns Draft/Published on in the same edit therefore emitted a read of a `_status` the old companion never had, into a `status` the main table has not been given yet — a disable deliberately runs the companion transition before the shared ALTER that adds it. The migration fails after it may already have re-added the localized columns, while the registry has recorded the entity as no longer localized. The verdict now comes from the caller, which knows both physical shapes: the existing companion's `_status` and the main table's `status`. Omitted means leave status alone. This is the same mistake the runtime restore already carries a comment about — that the desired schema cannot answer whether the columns are there — reproduced in the generator while fixing an unrelated gap in it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 557394fe6a
ℹ️ 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".
…ot be undone The fact that a re-enable must OVERWRITE a surviving companion lives in the state it claims from — `restored` — and claiming replaces that state with `enabling`. A run that crashed in between left a marker indistinguishable from an ordinary unfinished seed, so the retry did the guarded insert, skipped the stale rows, settled, and left them hiding every edit made while localization was off. The claim records what it owes now, a takeover carries it rather than downgrading it, and settling clears it because the work is done. The restore copy carries its transition into the statement, as the re-enable refresh already did. Two processes disabling one entity can both read the same marker and reach it; if the first finishes and publishes the non-localized configuration, edits land on main and the second overwrites them from a companion that is stale by then. Noticing the lost record afterwards cannot bring those edits back. Restoring an untracked companion no longer needs a configured default. Removing the localization block outright is when an entity has neither a record nor a default — and the copy stopped needing a locale named once it began ranking each parent's own rows, so all that was missing was something true to write in the record. A locale the companion demonstrably holds is that. The all-locale component overlay returned before the containment the single-locale branch had, so a drifted companion still cost the reader the whole component rather than its translations. Both now run through one helper. A metadata update that drops a companion forgets its readiness, as the dispatcher disable paths already do. And losing the CREATE race after claiming the transition is reported rather than read as a quiet non-creation: this run holds the marker for a table another made, that run may have died before seeding it, and a caller told nothing lets the apply drop the columns whose values never got across.
This was referenced Jul 31, 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
1 participant
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 task 078 and task 086, and works the eleven review threads left open when #419 merged.
Why one PR
086's own analysis concluded it should not be fixed on its own: the read path aborts a PostgreSQL transaction because it decides whether a companion exists by running the join and catching the failure, and the only fix that does not add a fourth mechanism for one fact is 078's readiness. So they land together, in that order.
The carried #419 findings
Each was proven to fail without its fix, and the observed message is quoted in the commit that fixes it.
localized: falseread back the stale retained column.expected [{title: "Before localizing"}] to deeply equal [{title: "Edited in English"}]UPDATEwith no guard assigns SQL NULL.expected [{title: null}] to deeply equal [{title: 'Kept'}]no such column: "status"— and because the companion was already created, every retry hit the same statement, so localization + Draft/Published in one edit could never applyuntracked; the loser still recorded the language.recorded: 'en', received: 'de'ADD COLUMN.expected [] to have a length of 1DROPTwo things the review had not found, both discovered while fixing the first:
localizationblock — which is exactly when it is owed. The store is now resolved without requiring a configured default locale, and the recorded source locale stands in.restoredstate, and re-enabling overwrites those rows from main.Readiness (078)
Three consumers asked one question — provisioning, the write guard, the read-back — each its own way, per write. An entry whose dynamic zone holds K localized field-group types paid
1 + Kround trips before its transaction and K more inside it.Three states, not a boolean, because "no companion" splits into a legitimate main-table fallback and a state that must be refused, and a boolean would leave the introspection that separates them on the write path.
Only
readyis remembered. Task 078 proposed caching everything, and its own correction 2 admits that widens the window where an out-of-processdb:sync/migrateleaves a running server acting on a stale answer. That trade is unnecessary:readyis the healthy steady state where the whole per-write cost lives, and it is reached by creating a table, which no ordinary operation undoes. The abnormal states re-resolve every time, so an entity mid-transition keeps exactly the freshness it has today.The read-path abort (086)
companion-jointolerated a missing companion at five sites by catching the failed query. That is a valid existence check on SQLite and MySQL and a transaction-killer on PostgreSQL, and several of those reads run inside the caller's write transaction — so the check caused the failure it was written to tolerate.Readiness is now a required argument on every reader, so a new caller cannot omit it and quietly reintroduce the blind join. Nothing in that module catches: not ready means no query, ready means every failure propagates. The
strictflag existed only to opt out of the swallow and goes with it.086's reproduction test, removed from #382 because it could not pass, is restored — and it now runs against a suite that can see the failure, since #412's guard fails any run that leaves a transaction poisoned.
The presence map #382 threaded from a pre-transaction pass through three services collapses into the same lookup.
Surfaces
nextly db:sync, a development tool that cannot help there because boot deliberately refuses to run DDL.nextly migratebecomes the remedy it names: it provisions companions after the migrations run, and repairs installs that transitioned before transitions were recorded. Absence is read as a debt only under supervision — an entity localized from birth is untracked too and owes nothing.Verification
Not in this PR
Gating the admin locale switcher on readiness. It needs a new API surface to carry readiness to the client, it is purely additive UX with no correctness dependency on any of the above, and this PR is already large enough that review tooling starts skipping files. Better as a focused follow-up.
One more instance of the catch-based existence check remains, on a different table:
field-group-query-servicestill tolerates a missingcomp_*main table that way. Closing it needs readiness for main tables rather than companions, which is its own piece of work; the matcher is now local to that file with a comment saying so.Summary by CodeRabbit
New Features
Bug Fixes