fix(nextly): stop localized writes overwriting default-locale content - #382
fix(nextly): stop localized writes overwriting default-locale content#382mobeenabdullah wants to merge 11 commits into
Conversation
|
@codex please review this PR |
|
Warning Review limit reached
Next review available in: 36 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 (16)
📝 WalkthroughWalkthroughThe PR adds localized companion-table provisioning to CLI, watcher, and boot flows, seeds existing default-locale data, rejects unsafe localized writes when companions are missing, and fixes webhook recording generation resets with regression coverage. ChangesLocalized companion lifecycle
Webhook recording generation
Estimated code review effort: 4 (Complex) | ~60 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)
🔧 Fix failing CI
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: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe76865f50
ℹ️ 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".
|
@codex please review this PR |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/nextly/src/cli/commands/__tests__/db-sync-localized-companion.integration.test.ts (1)
157-181: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering a localized component too.
Components are the one kind whose
dbNameis honored verbatim (resolveComponentTableName), which is exactly the divergence thedbNametest guards against for collections — but no case exercises the component branch ofensureLocalizedCompanions.🤖 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/cli/commands/__tests__/db-sync-localized-companion.integration.test.ts` around lines 157 - 181, Extend the localized companion integration coverage to include a localized component with a custom dbName, exercising the component branch of ensureLocalizedCompanions. Use resolveComponentTableName semantics by asserting the companion table is derived from the verbatim component dbName, and verify the incorrectly prefixed alternative is absent.packages/nextly/src/domains/i18n/runtime/companion-io.ts (1)
246-252: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc block still claims failures are swallowed.
The behavior now reports through the optional
onError(Lines 269–273, 312–322); "a failure ... is swallowed so it retries on the next boot" only holds for callers that omit the reporter. Worth qualifying so the contract reads consistently.🤖 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/runtime/companion-io.ts` around lines 246 - 252, Update the documentation for the companion creation flow near its best-effort behavior to clarify that failures are reported through the optional onError callback when provided, while callers omitting onError swallow the failure and retry on the next boot. Keep the existing runtime behavior unchanged.
🤖 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/di/register.ts`:
- Around line 476-479: Update the initializeSchemaRegistry call in the
surrounding registration flow to source localization.defaultLocale from
transformedConfig instead of config, preserving optional access. Align this
registry path with the existing ensureCompanionTable call sites that use
transformedConfig so plugin-transformed locale values are passed through.
In
`@packages/nextly/src/domains/i18n/__tests__/localized-write-without-companion.integration.test.ts`:
- Around line 12-15: Remove the sentence referencing “published alpha.43” and
“task 006” from the header comment in
localized-write-without-companion.integration.test.ts. Keep the preceding
explanation of the non-default-locale failure mode unchanged.
---
Nitpick comments:
In
`@packages/nextly/src/cli/commands/__tests__/db-sync-localized-companion.integration.test.ts`:
- Around line 157-181: Extend the localized companion integration coverage to
include a localized component with a custom dbName, exercising the component
branch of ensureLocalizedCompanions. Use resolveComponentTableName semantics by
asserting the companion table is derived from the verbatim component dbName, and
verify the incorrectly prefixed alternative is absent.
In `@packages/nextly/src/domains/i18n/runtime/companion-io.ts`:
- Around line 246-252: Update the documentation for the companion creation flow
near its best-effort behavior to clarify that failures are reported through the
optional onError callback when provided, while callers omitting onError swallow
the failure and retry on the next boot. Keep the existing runtime behavior
unchanged.
🪄 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: 88abd0d2-28d6-45aa-af92-7a0235dae530
⛔ Files ignored due to path filters (1)
.changeset/localized-write-companion-guard.mdis excluded by!.changeset/**
📒 Files selected for processing (12)
packages/nextly/src/cli/commands/__tests__/db-sync-localized-companion.integration.test.tspackages/nextly/src/cli/commands/db-sync.tspackages/nextly/src/cli/commands/dev-build.tspackages/nextly/src/cli/commands/dev-watcher.tspackages/nextly/src/di/register.tspackages/nextly/src/domains/collections/services/collection-mutation-service.tspackages/nextly/src/domains/i18n/__tests__/localized-write-without-companion.integration.test.tspackages/nextly/src/domains/i18n/migration/generate-up.tspackages/nextly/src/domains/i18n/runtime/companion-io.tspackages/nextly/src/domains/singles/services/single-mutation-service.tspackages/nextly/src/domains/webhooks/__tests__/recording-policy.test.tspackages/nextly/src/domains/webhooks/recording-policy.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38766c3381
ℹ️ 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".
|
@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: af53dbca1a
ℹ️ 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".
af53dbc to
61b30b4
Compare
|
@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: 61b30b4bac
ℹ️ 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".
|
@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: 30692efb2a
ℹ️ 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".
30692ef to
43eaf63
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43eaf6381e
ℹ️ 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".
|
@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: ba79cfee62
ℹ️ 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".
| await adapter.executeQuery( | ||
| `UPDATE ${companion} SET ${q(column)} = ` + | ||
| `(SELECT ${q(column)} FROM ${main} WHERE ${main}.${q("id")} = ${companion}.${q("_parent")}) ` + | ||
| `WHERE ${companion}.${q("_locale")} = ${placeholder}`, | ||
| [args.defaultLocale] |
There was a problem hiding this comment.
Create missing default-locale rows during field backfill
This UPDATE only backfills parents that already have a default-locale companion row. A document created solely in a non-default locale legitimately has no such row; if a previously shared main-table field is later marked localized, its new companion column remains null for that document and the main value disappears from localized reads. Backfill with a Drizzle insert/upsert that also creates missing default-locale rows while preserving existing translations.
AGENTS.md reference: AGENTS.md:L83-L85
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged, and not fixed — I would rather say that plainly than leave it looking handled.
You are right on the mechanism, and it is a consequence of my own previous fix, which is the reason I have stopped rather than continuing to patch. This is the sixth review round on this PR, and four of the six findings in it are regressions from fixes I made in the two rounds before — including the production ALTER TABLE path, which I introduced while fixing companion reconciliation.
The write-guard half of this PR has been stable since the first round. The seeding/transition half is an unattended data migration that has to infer state nobody recorded, and it has grown a new failure mode at a new seam in every round since it was added.
I have asked the maintainer to decide between landing the guard and moving the seeding work to its own PR, or continuing here. Reverting that scope is not my call to make unilaterally, so this thread stays open until they choose. If the answer is to continue here, this is one of the four I will pick up.
There was a problem hiding this comment.
Confirmed, and not fixed — flagged rather than guessed at.
A document created only in a non-default language legitimately has no default-locale row, so the UPDATE has nothing to touch and a newly localized field's shared value never reaches it.
The insert half is straightforward on its own. What makes me want to do this deliberately rather than inline is that it interacts with the retryability question on the neighbouring thread and with the seed's "companion must be empty" rule: all three are the same underlying problem, which is that nothing records whether an entity's localization transition has completed, so each of these paths has to infer it from physical shape. Every inference this PR has added has needed a correction in the next review round.
Flagged to the maintainer as one of two remaining items.
ba79cfe to
17c97d7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17c97d7eed
ℹ️ 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".
| .filter( | ||
| column => !companionColumns.has(column) && mainColumns.has(column) | ||
| ); |
There was a problem hiding this comment.
Make companion-column backfills retryable
If reconciliation successfully adds a companion column but backfillAddedCompanionColumns then fails—for example, due to a transient connection error or after the first of several column updates—the helper swallows the failure and a retry sees the column in companionColumns, so this filter permanently excludes it from backfill. Existing shared values remain on the main table but localized reads continue returning null for that field; track completion separately or make the add-and-backfill operation recoverable on subsequent runs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and not fixed — flagging it rather than guessing.
The gap is real: if the column ADD succeeds and the backfill then fails, a retry sees the column in companionColumns and permanently excludes it, so the shared value stays on the main table while localized reads keep returning null.
What stops me shipping a fix is that the obvious retry conditions are unsafe in a way that matters. "Backfill whenever the default-locale value is null" would resurrect content a user had deliberately cleared, because once localized, writes go to the companion and the main column keeps its old value indefinitely. Distinguishing "never backfilled" from "cleared on purpose" needs state that nothing currently records.
Doing it properly means tracking transition completion explicitly rather than inferring it from the physical shape — which is the same conclusion your defaultLocale finding pushed the seed towards. I would rather do that as a deliberate piece of work than add another inference here; this PR has already had several fixes that moved a problem rather than solved it.
Flagged to the maintainer as one of two remaining items.
17c97d7 to
3a0c3f4
Compare
|
@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: 3a0c3f4f1b
ℹ️ 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".
| await adapter.executeQuery( | ||
| `UPDATE ${companion} SET ${q(column)} = ` + | ||
| `(SELECT ${q(column)} FROM ${main} WHERE ${main}.${q("id")} = ${companion}.${q("_parent")}) ` + | ||
| `WHERE ${companion}.${q("_locale")} = ${placeholder}`, |
There was a problem hiding this comment.
Route the backfill through Drizzle
This new runtime backfill assembles an UPDATE statement manually and sends it through executeQuery, bypassing the typed table descriptors and reimplementing dialect-specific quoting. Product database access in this repository is required to use Drizzle exclusively, so express this update with the companion and main Drizzle table objects instead.
AGENTS.md reference: AGENTS.md:L83-L85
Useful? React with 👍 / 👎.
| for (const stmt of statements) { | ||
| await adapter.executeQuery(stmt); | ||
| } |
There was a problem hiding this comment.
Backfill each added column before later DDL can fail
When reconciling an existing companion with multiple newly localized columns, every ALTER runs before any backfill; if a later statement or backfill fails after an earlier ADD COLUMN committed—particularly on MySQL—the catch returns successfully. On the next run, introspection treats that column as pre-existing and excludes it from added, so its shared main-table value is never copied and localized reads can remain null. Pair each additive statement with its backfill or otherwise retain enough state to retry incomplete backfills.
Useful? React with 👍 / 👎.
| // - before the `!hasChanges` return, because that drop is classified unsafe and | ||
| // deferred, which leaves `hasChanges` false — so the exact transition this | ||
| // exists to support would return early and never provision anything. | ||
| await ensureLocalizedCompanionsForReload(adapter, newConfig); |
There was a problem hiding this comment.
Retry companion creation after HMR creates the main table
When an entirely new localized collection, single, or field group is added during HMR, this call runs before the schema pipeline creates its main table, so the companion's foreign-key creation fails and is swallowed by ensureCompanionTable. The later apply creates the main table, but this helper is never called again, while metadata and runtime schemas are still published as localized; non-default writes are therefore refused until another reload or restart. Keep the pre-apply call for seeding transitions, but run the idempotent provisioning again after a successful apply.
AGENTS.md reference: packages/nextly/AGENTS.md:L36-L40
Useful? React with 👍 / 👎.
Closes the P0 found on published
0.0.2-alpha.43during the alpha demo walk: enabling localization on a collection that already had content could silently destroy the default language.The defect
ensureCompanionTableis the intended db:sync/dev-boot counterpart to migration-owned companion creation, and it works — but it was only ever called at boot, fromdi/register.ts.nextly db:syncruns in a separate CLI process. It flipsdynamic_collections.<slug>.localizedto1and returns. So in the window between that and the next boot:localized: 1, registersdc_posts_locales, and renders the complete localization UI — content-language switcher, "Shared" badges,Default:fallback hints, all correct;dc_posts_localestable does not exist;Measured on alpha.43:
Every surface reports success: the CLI says "Schema applied — no changes", the admin renders a correct localization UI, and the save succeeds. The user believes they added a translation; they replaced their original-language content and changed its public URL.
The fix
1. Close the window. A new
ensureLocalizedCompanionscreates the_localescompanion for every localized collection, single and component, called fromdb-sync.tsand fromdev-watcher.ts(turning localization on is a config edit, so it arrives through HMR as often as through the CLI). It must run after all three syncs — a companion carries a foreign key to its main table, so running it earlier creates nothing for singles and components. Idempotent: one probe per localized entity.2. Fail closed anyway. If the companion is somehow still missing, a non-default-locale update is refused with
NextlyError.conflictand an actionable message rather than falling through to the main row. Collections and singles both (singles fail differently — the split drops the translation silently instead of overwriting).Scoped to updates on purpose. A non-default-locale create legitimately writes to the main table until the companion exists — a new entry has no other language's values to lose — and that is the documented pre-migration fallback, asserted by
writes-create.integration.test.ts.3. Stop swallowing failures.
ensureCompanionTablehad a blanketcatch {}, so a persistent failure was indistinguishable from first-boot ordering. It takes an optionalonErrorreporter;db:synclogs a warning naming the entity and the consequence. Existing callers are unchanged.Also included: a leftover P2 from #375 —
resetWebhookRecordingPolicyzeroed the refresh generation, so a refresh still in flight could match a later boot's counter and republish a stale decision (ABA). The counter now only increases.Verification
Every test below was run with its fix disabled and observed to fail.
esupdate)expected true to be false— the write silently succeeded and overwroteexpected 500 to be 409— an opaque driver error, not our refusaldb:synccreates a localized collection's companionexpected false to be truedb:synccreates a localized single's companionexpected false to be trueresetWebhookRecordingPolicyABAOnly the two-boot case demonstrates data loss, because it is the only one where the translatable value starts on the main table. The dialect-parametrized cases prove the guard fires everywhere, which is the part that is dialect-sensitive:
companionTableExistsprobes with dialect-quoted SQL, and on Postgres a failed statement inside a transaction poisons the rest of it.check-types0 errors,lintclean.Not in this PR
localized: 1with no companion the wayfindCoreSchemaDriftreports missing columns. Item 4 of the task's fix list; stays open as a follow-up.db-sync.tsanddev-watcher.tsis not covered by a test; the test drives the sequence directly.migrate:createemitting a full schema on db:sync-managed projects, which is why the migration route was not a usable workaround during the walk. Filed separately.Summary by CodeRabbit
New Features
Bug Fixes