feat(nextly): execute and verify the field group storage migration's steps - #423
Conversation
|
@codex please review this PR Three things I would most like challenged:
@coderabbitai review |
|
Warning Review limit reached
Next review available in: 43 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 (31)
📝 WalkthroughWalkthroughField-group migration markers now carry canonical applied plans and separate registry and manifest identities. New observable, idempotent migration steps, SQL-backed locking, sync exclusions, reload guards, and schema-scoped index introspection are covered by tests. ChangesField-group migration safety
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant dbSync
participant withMigrationExcluded
participant MigrationSession
participant Database
dbSync->>withMigrationExcluded: run synchronization workflow
withMigrationExcluded->>MigrationSession: acquire exclusion and check migration marker
MigrationSession->>Database: claim lock row
withMigrationExcluded->>dbSync: execute sync while exclusion is held
MigrationSession->>Database: release owned lock
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
@nextlyhq/adapter-drizzle
@nextlyhq/adapter-mysql
@nextlyhq/adapter-postgres
@nextlyhq/adapter-sqlite
@nextlyhq/admin
@nextlyhq/admin-css
@nextlyhq/blocks-engine
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: |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Changed files ==='
git diff --name-only "$(git merge-base HEAD origin/main)" HEAD 2>/dev/null || git diff --name-only HEAD~1 HEAD
printf '%s\n' '=== Candidate migration files ==='
fd -t f . | rg -i '(field.*group|group.*storage|migration|manifest|snapshot)' | head -n 200
printf '%s\n' '=== Relevant symbol locations ==='
rg -n -C 3 \
'assertPlanUnchanged|parseAppliedManifest|introspectLiveSnapshot|quoteIdent|slug.*hash|hash.*slug|registry.*pointer|table_name|not comparable|comparable|resume|stored plan|storedPlan' \
. -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**' \
| head -n 500Length of output: 142029 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="$(git merge-base HEAD origin/main 2>/dev/null || git rev-parse HEAD~1)"
printf '%s\n' '=== Module outlines ==='
for f in \
packages/nextly/src/domains/field-groups/migration/{guard,manifest,observer,state,steps,sync-guard}.ts
do
echo "--- $f"
ast-grep outline "$f" --items all --view expanded || true
done
printf '%s\n' '=== Changed production diff ==='
git diff --unified=80 "$base" HEAD -- \
packages/nextly/src/domains/field-groups/migration/guard.ts \
packages/nextly/src/domains/field-groups/migration/manifest.ts \
packages/nextly/src/domains/field-groups/migration/observer.ts \
packages/nextly/src/domains/field-groups/migration/state.ts \
packages/nextly/src/domains/field-groups/migration/steps.ts \
packages/nextly/src/domains/field-groups/migration/sync-guard.ts \
packages/nextly/src/cli/commands/db-sync.ts
printf '%s\n' '=== Focused test names and assertions ==='
rg -n -C 5 \
'describe\(|it\(|test\(|assertPlanUnchanged|parseAppliedManifest|duplicate|slug|resume|index|comparable|registry|table_name' \
packages/nextly/src/domains/field-groups/migration/__tests__/{manifest,observer,runner,steps,guard,sync-guard}.test.tsLength of output: 214953 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Precise call sites ==='
rg -n -C 8 \
'\b(buildMigrationSteps|assertPlanUnchanged|findLostIndexes|refuseLostIndexes|createStorageObserver|beginMigration|runMigrationSteps|hashSlugSet)\s*\(' \
packages/nextly/src/domains/field-groups \
-g '*.ts'
printf '%s\n' '=== Persisted-plan parser and resume assertion ==='
sed -n '430,620p' packages/nextly/src/domains/field-groups/migration/state.ts
printf '%s\n' '=== Migration orchestration outlines ==='
for f in \
packages/nextly/src/domains/field-groups/migration/{reconcile,runner,session}.ts
do
echo "--- $f"
ast-grep outline "$f" --items all --view expanded
done
printf '%s\n' '=== Orchestration source ==='
for f in \
packages/nextly/src/domains/field-groups/migration/{reconcile,runner,session}.ts
do
echo "--- $f"
cat -n "$f"
done
printf '%s\n' '=== Relevant test source around state/resume ==='
rg -n -C 8 \
'same slug|tableName|slug.*hash|hash.*slug|assertPlanUnchanged|duplicate.*registry|registry.*twice|appliedManifest|persisted plan' \
packages/nextly/src/domains/field-groups/migration/__tests__/{state,manifest,guard,steps}.test.tsLength of output: 244151
The persisted-plan approach itself is sound, and deferring the resume-only index comparability gap to B1-5 is reasonable. The first finding is broader, though: the index check needs wiring before B1-5 can serve as a backstop. ✅ Action performedReview finished.
|
![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: 81c20ca9dd
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/nextly/src/domains/field-groups/migration/__tests__/state.test.ts (1)
657-661: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
refusalFrom's parameter type still names the removedplanHashfield.The helper at Line 637 declares
current: { manifestHash: string; planHash: string }, so this new call (which passesslugsHash/manifestHash) no longer matches theMigrationPlanIdentitycontractassertPlanUnchangedaccepts. It runs fine because the spread supplies the right keys at runtime, and*.test.tsis excluded fromcheck-types, but the stale signature hides the shape change.♻️ Align the helper with the new identity shape
- function refusalFrom(current: { - manifestHash: string; - planHash: string; - }): NextlyError { + function refusalFrom(current: MigrationPlanIdentity): NextlyError {🤖 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/migration/__tests__/state.test.ts` around lines 657 - 661, Update the refusalFrom helper’s current parameter type to use the new MigrationPlanIdentity shape, replacing the removed planHash field with slugsHash alongside manifestHash. Ensure its argument is compatible with the identity passed to assertPlanUnchanged while preserving the existing refusal behavior and tests.Source: Learnings
🤖 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/db-sync.ts`:
- Around line 289-292: Remove the redundant `as unknown as DrizzleAdapter`
assertion from the `adapter` argument in the `assertNoMigrationInFlight` call,
passing the declared `adapter` value directly while preserving the existing
logger and migration-check behavior.
---
Nitpick comments:
In `@packages/nextly/src/domains/field-groups/migration/__tests__/state.test.ts`:
- Around line 657-661: Update the refusalFrom helper’s current parameter type to
use the new MigrationPlanIdentity shape, replacing the removed planHash field
with slugsHash alongside manifestHash. Ensure its argument is compatible with
the identity passed to assertPlanUnchanged while preserving the existing refusal
behavior and tests.
🪄 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: dc425e17-f595-4fde-8ed2-f3bb4f5ee210
⛔ Files ignored due to path filters (1)
.changeset/field-group-migration-ddl.mdis excluded by!.changeset/**
📒 Files selected for processing (15)
packages/nextly/src/cli/commands/db-sync.tspackages/nextly/src/domains/field-groups/migration/__tests__/guard.test.tspackages/nextly/src/domains/field-groups/migration/__tests__/manifest.test.tspackages/nextly/src/domains/field-groups/migration/__tests__/observer.test.tspackages/nextly/src/domains/field-groups/migration/__tests__/runner.test.tspackages/nextly/src/domains/field-groups/migration/__tests__/state.test-d.tspackages/nextly/src/domains/field-groups/migration/__tests__/state.test.tspackages/nextly/src/domains/field-groups/migration/__tests__/steps.test.tspackages/nextly/src/domains/field-groups/migration/__tests__/sync-guard.test.tspackages/nextly/src/domains/field-groups/migration/guard.tspackages/nextly/src/domains/field-groups/migration/manifest.tspackages/nextly/src/domains/field-groups/migration/observer.tspackages/nextly/src/domains/field-groups/migration/state.tspackages/nextly/src/domains/field-groups/migration/steps.tspackages/nextly/src/domains/field-groups/migration/sync-guard.ts
|
Correcting a claim in this PR description before working the round. The description and the changeset both say index survival is checked by name. It is not. Two more I have already verified as real rather than taking on trust:
I am treating the remaining six as unverified until I check each against the code, and will reply per thread with either a fix proven by breaking its test, or a rebuttal with file:line evidence. No merge request here — this PR is not ready. |
|
Round 1 status at Fixed (five), each proven load-bearing by breaking it:
Rebutted (one), with evidence: the Three still open, and I want to be straight that two need design rather than a patch:
I would rather land these as considered changes than as quick patches, so they are the next work on this PR, not this push. @codex please review this PR |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="$(git merge-base HEAD origin/main 2>/dev/null || git rev-parse HEAD~1)"
printf '%s\n' '=== HEAD ==='
git rev-parse --short HEAD
git status --short
printf '%s\n' '=== Relevant changed files ==='
git diff --name-only "$base" HEAD -- \
packages/nextly/src/domains/field-groups/migration \
packages/nextly/src/cli/commands/db-sync.ts
printf '%s\n' '=== Outlines ==='
for f in \
packages/nextly/src/domains/field-groups/migration/{manifest,observer,steps,state,runner,sync-guard}.ts \
packages/nextly/src/cli/commands/db-sync.ts
do
if [ -f "$f" ]; then
echo "--- $f"
ast-grep outline "$f" --items all --view expanded || true
fi
done
printf '%s\n' '=== Focused definitions and calls ==='
rg -n -C 7 \
'\b(findLostIndexes|refuseLostIndexes|indexNames|registryNameAt|buildMigrationSteps|assertPlanUnchanged|hashSlugSet|createDebouncedSync|assertNoMigrationInFlight|inTransaction)\s*\(' \
packages/nextly/src/domains/field-groups/migration \
packages/nextly/src/cli/commands/db-sync.ts \
-g '*.ts'
printf '%s\n' '=== Updated production diff ==='
git diff --unified=50 "$base" HEAD -- \
packages/nextly/src/domains/field-groups/migration \
packages/nextly/src/cli/commands/db-sync.ts
printf '%s\n' '=== Focused tests ==='
rg -n -C 5 \
'placeholder|\\$1|pool|transaction|satisfied|pointer|index|case|companion|watch|debounced|same.slug|replacement|registry' \
packages/nextly/src/domains/field-groups/migration/__tests__ \
packages/nextly/src/cli \
-g '*test*.ts' || trueLength of output: 202048 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== steps.ts ==='
cat -n packages/nextly/src/domains/field-groups/migration/steps.ts
printf '%s\n' '=== session.ts ==='
cat -n packages/nextly/src/domains/field-groups/migration/session.ts
printf '%s\n' '=== db-sync orchestration ==='
rg -n -C 18 \
'function runDbSync|const runDbSync|assertNoMigrationInFlight|createDebouncedSync|syncCollections|syncSingles|syncComponents|watchConfig' \
packages/nextly/src/cli/commands/db-sync.ts
printf '%s\n' '=== migration orchestration callers ==='
rg -n -C 12 \
'\b(buildMigrationSteps|runMigrationSteps|withMigrationSession|assertPlanUnchanged)\s*\(' \
packages/nextly/src/domains/field-groups \
-g '*.ts'
printf '%s\n' '=== Exact focused test ranges ==='
sed -n '45,140p' packages/nextly/src/domains/field-groups/migration/__tests__/steps.test.ts
sed -n '185,410p' packages/nextly/src/domains/field-groups/migration/__tests__/steps.test.ts
sed -n '150,185p' packages/nextly/src/domains/field-groups/migration/__tests__/session.test.ts
printf '%s\n' '=== Placeholder implementation across adapters ==='
rg -n -C 8 \
'placeholder|\\$[0-9]|client\.query|executeQuery' \
packages/nextly/src/database \
packages/adapter-postgres \
-g '*.ts' || trueLength of output: 184144 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Storage observer implementation ==='
cat -n packages/nextly/src/domains/field-groups/migration/observer.ts
printf '%s\n' '=== Debounced watch sync implementation ==='
fd -t f '^dev-watcher\.ts$' packages/nextly/src/cli --exec sh -c 'echo "--- $1"; cat -n "$1"' sh {}
printf '%s\n' '=== Production step-builder call sites ==='
rg -n -C 16 \
'\bbuildMigrationSteps\s*\(' \
packages/nextly/src \
-g '*.ts' -g '!**/__tests__/**'
printf '%s\n' '=== Registry parser multiplicity validation ==='
rg -n -C 14 \
'parseAppliedManifest|exactly one|registry.*entry|entry.*registry' \
packages/nextly/src/domains/field-groups/migration \
-g '*.ts'
printf '%s\n' '=== Column-step pool/re-entry coverage ==='
rg -n -C 10 \
'column.*pool|pool.*column|observer\.columns|inTransaction' \
packages/nextly/src/domains/field-groups/migration/__tests__ \
-g '*.ts'Length of output: 61832
Two findings remain:
The three issues you listed as deliberately open—same-slug row replacement, companion ordering, and watch-mode re-sync—remain accurately characterized by the code. In particular, ✏️ Learnings added
✅ Action performedReview finished.
|
fc4aa22 to
b190ae6
Compare
|
Round 1 complete at Since the last status: companion renames now move with their owner and the pointer in one transaction (no ordering fixes it alone — the pointer decides which companion name the registry derives, so it can only move when both objects have); the marker is re-checked on every watched re-sync rather than once at boot; and resume identity now covers each registry row's primary key, not just its slug. Every fix has a test proven load-bearing by breaking it. @codex please review this PR |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="$(git merge-base HEAD origin/main 2>/dev/null || git rev-parse HEAD~1)"
echo '=== HEAD and changed files ==='
git rev-parse --short HEAD
git diff --name-only "$base" HEAD -- \
packages/nextly/src/domains/field-groups/migration \
packages/nextly/src/cli/commands/db-sync.ts \
packages/nextly/src/cli
echo '=== Module outlines ==='
for file in \
packages/nextly/src/domains/field-groups/migration/steps.ts \
packages/nextly/src/domains/field-groups/migration/state.ts \
packages/nextly/src/domains/field-groups/migration/runner.ts \
packages/nextly/src/domains/field-groups/migration/session.ts \
packages/nextly/src/domains/field-groups/migration/sync-guard.ts \
packages/nextly/src/cli/commands/db-sync.ts
do
if [ -f "$file" ]; then
echo "--- $file"
ast-grep outline "$file" --items all --view expanded || true
fi
done
echo '=== Relevant definitions, call sites, and tests ==='
rg -n -C 8 \
'\b(buildMigrationSteps|registryNameAt|assertPlanUnchanged|hashSlugSet|hash.*Identity|primary.?key|row.*id|assertNoMigrationInFlight|createDebouncedSync|inTransaction|observer\.columns|companion|pointer)\s*\(' \
packages/nextly/src/domains/field-groups/migration \
packages/nextly/src/cli \
-g '*.ts'
echo '=== Updated production diff ==='
git diff --unified=60 "$base" HEAD -- \
packages/nextly/src/domains/field-groups/migration \
packages/nextly/src/cli/commands/db-sync.ts \
packages/nextly/src/cli/dev-watcher.ts
echo '=== Focused test references ==='
rg -n -C 5 \
'companion|atomic|transaction|watch|debounced|primary.?key|row.*id|same.slug|replacement|duplicate.*registry|pool|max.?1|column' \
packages/nextly/src/domains/field-groups/migration/__tests__ \
packages/nextly/src/cli \
-g '*test*.ts' || trueLength of output: 202305 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Current commit ==='
git rev-parse --short HEAD
echo '=== steps.ts: complete focused implementation ==='
sed -n '1,430p' packages/nextly/src/domains/field-groups/migration/steps.ts
echo '=== manifest and state identity implementation ==='
rg -n -C 12 \
'export function hashRegistryIdentity|export function hashSlugSet|interface RegistryRow|type RegistryRow|assertPlanUnchanged|registryIdentity' \
packages/nextly/src/domains/field-groups/migration/manifest.ts \
packages/nextly/src/domains/field-groups/migration/state.ts
echo '=== watch-mode and db-sync paths ==='
rg -n -C 25 \
'assertNoMigrationInFlight|createDebouncedSync|runDbSync|watchConfig|syncCollections|syncSingles|syncComponents|ensureLocalizedCompanions' \
packages/nextly/src/cli/commands/db-sync.ts \
packages/nextly/src/cli/dev-watcher.ts \
packages/nextly/src/cli \
-g '*.ts'
echo '=== Direct production builder call sites ==='
rg -n -C 15 '\bbuildMigrationSteps\s*\(' packages/nextly/src -g '*.ts' -g '!**/__tests__/**'
echo '=== Regression test bodies ==='
sed -n '390,475p' packages/nextly/src/domains/field-groups/migration/__tests__/steps.test.ts
rg -n -C 15 'watch|re-sync|in.flight|assertNoMigrationInFlight|migration marker' \
packages/nextly/src/cli \
packages/nextly/src/domains/field-groups/migration/__tests__ \
-g '*test*.ts'
rg -n -C 15 'same slug|recreated|registry identity|hashRegistryIdentity|assertPlanUnchanged' \
packages/nextly/src/domains/field-groups/migration/__tests__ \
-g '*test*.ts'Length of output: 162326 @mobeenabdullah I verified the new watch-mode guard and the row-ID-based resume identity; both are present and address their stated failure modes. However, two previously reported issues remain in
The grouped owner/companion/pointer operation, the watched re-sync guard, and registry identity change otherwise look correctly wired. ✅ Action performedReview finished.
|
A resume rebuilt its plan from registry rows and compared the result against two recorded hashes. That stops working as soon as each rename updates its registry pointer: a rebuilt plan then omits every row already renamed, so it has fewer entries, different step positions, and a different hash from the one the marker recorded, and the resume refuses the plan it is resuming. The plan is now persisted in the marker for both directions and read back rather than rebuilt. It is always the canonical legacy-to-migrated form: an up run applies it, a down run reverses it at execution time. A run in flight without one is corruption rather than an optional field, and the resume verdict carries it in both directions. What the identity has to catch changes with it. The stored plan's integrity is checked on read against a hash of itself, and the question a recorded step position actually depends on — did the set of field groups move underneath the interrupted run — is answered by hashing the sorted slug set. Slugs rather than table names, because table names are rewritten as the run progresses, so a name-based comparison would refuse every resume past the first step. The step- list hash retires with the rebuild that needed it. Three tests were passing for a reason other than their own: the marker they built was refused by the new plan checks before reaching the guard each was written to exercise. They now carry a valid plan, and the step-bound test fails again when its guard is removed.
Renaming a table and updating the registry row that points at it are one logical change. Apart, there is a window in which every row addresses a table that no longer exists, and the read path turns that into empty content rather than an error. Each rename step now issues both in one transaction, and its postcondition checks both: a rename whose pointer update did not land fails verification rather than being recorded as done. Which name the pointer update targets is derived from the plan rather than probed for. The registry renames last going up, so every update precedes it and addresses the legacy name; a rollback reverses that order, so the registry moves first and later steps address the name the rollback restored. Reading it from the catalog instead would repeat a judgement reconciliation already owns, and mid-run both names can be observable on a server that folds case. Both halves are idempotent because a step has to survive being half applied. MySQL commits DDL implicitly, so a rename can land while the pointer update fails; the resume re-runs the step, finds the table already moved, and applies only the half that is missing. Companions carry no pointer of their own — their name derives from the owner's table_name, so the owner's update already moves them — and no row addresses the registry itself. Both are still verified by name, because deriving a name is not evidence the object was renamed. No SQL is written here: generateSQL already emits both renames for all three dialects. The pointer update binds its values and quotes only the table name, which cannot go through Drizzle because the schema registry resolves tables by the exact name their definition declares and a migrated table is unregistered as far as it is concerned.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d2ef2f26f
ℹ️ 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".
Mid-run the database is in a shape neither the old config nor the new one describes: some tables carry their pre-rename names and some their post-rename names, and the registry rows pointing at them move one step at a time. db:sync reconciles the config against exactly that, and --remove-orphaned deletes what it cannot account for, so a sync during a partial migration can drop half-renamed storage. Nothing about that is recoverable by being careful inside the sync, so it refuses and names the run an operator has to clear. Placed after the core tables are ensured, because the marker lives in nextly_meta and a database that has never been set up has nothing to read, and before anything that inspects or changes schema. A marker that is present but unreadable refuses rather than reading as absent. Absence means no migration ever ran; an unreadable marker may still describe objects that were renamed, and treating the two alike would let the sync proceed over the storage this guard exists to protect.
…ction The steps decide against an injected observer so they stay testable without a database. This binds that seam to introspectLiveSnapshot rather than issuing new catalog queries: it already answers the same question for all three dialects, and a second implementation of what a table looks like would drift from it the way independent naming rules have drifted here before. Index survival is checked BY NAME. Renaming a table keeps its indexes on every supported dialect, so a name missing afterwards means one was dropped rather than moved. Comparing counts instead would survive losing one index and gaining another, which is the exact shape of the loss this guards against. An untracked index list stays undefined rather than collapsing to empty. Snapshots written before index data was recorded leave the field unset, and reading that as "no indexes" would report every index intact on a snapshot that never held any. The two cases are distinguished in the result, so a caller cannot mistake "nothing was lost" for "nothing could be compared". The registry pointer read addresses a table by a name the ORM's schema registry does not know, since it resolves tables exactly by the name their definition declares and the registry sits under whichever name the plan has reached.
…eck indexes Four defects found in review, all in the rename step. The pointer update emitted `?` markers on every dialect. node-postgres binds `$1`/`$2` and receives the statement unchanged, so every PostgreSQL rename step would have failed with a syntax error before renaming anything. Markers are now generated per dialect. The test double parsed `?` positionally and applied the parameters itself, which is why this passed; it now rejects anything the real driver would, and reverting the fix fails six tests. Observations moved out of the step transaction. The observer reads through the adapter, which takes its own connection, so asking it from inside the transaction waits for a second checkout and deadlocks a pool sized to one. A satisfied entry no longer skips its pointer repair. MySQL commits DDL implicitly, so reconciliation can mark a rename satisfied while its pointer update never landed; skipping both halves left the pointer stale and failed verification on every resume, with no way forward. The DDL is still skipped, and the update is idempotent. Index survival is now actually checked. The comparison helpers existed, were tested, and had no production caller — the step verified names and pointers only, so a rename that dropped an index still advanced the marker. `verify` now compares the names captured before the rename against those after, and refuses rather than returning false: a lost index is not work still to do, and retrying cannot bring it back.
… rules The snapshot lookup compared spellings exactly. MySQL with lower_case_table_names=1 answers a query for a mixed-case custom name while information_schema reports the lowercased one, so the lookup discarded a snapshot describing the very table it had asked about — and the caller then read an existing table as missing, refusing the migration over storage that is there. Resolved through the same identifier-case rules reconciliation already uses, so one set of rules decides what counts as the same object everywhere rather than two answers drifting apart.
A companion's name is derived from its owner's table_name, so the moment the pointer moves the registry begins deriving the new companion name. Renaming the companion in a separate, independently recorded step left a window in which that derived name addressed an object that did not exist yet, and a crash or a concurrent reader in that window saw localized storage missing. Neither ordering fixes it alone. Renaming the companion first leaves the pointer still deriving the old name for a table that has moved; renaming it after leaves the derived new name pointing at nothing. The pointer can only move when both objects have, so both renames and the pointer update now share one transaction. The companion keeps its own entry and position, so the plan's shape and hash are unchanged and step numbers still index it. Its own step finds the source already gone and does nothing, which is the same idempotence every step relies on.
The guard ran once, when db:sync started. A migration can begin after that, so every later save re-synced without it — and with --remove-orphaned, deleted what it could not account for, over tables halfway through being renamed. Placed beside the plugin-declaration gate in the watcher's sync, which already re-runs on every reload for the same reason: a check that only runs at boot protects the first sync and nothing after it.
A slug census cannot tell a field group that survived an interrupted run from one deleted and recreated under the same name. Recreate `hero` with an author-chosen dbName of `fg_hero` and the hash is unchanged, so a resume reads that table as its own crash-window work, settles claiming it created it, and a later rollback renames the author's table to `comp_hero`. The identity now covers each registry row's own primary key alongside its slug. The id is stable across the run — renames rewrite table_name, never this — and a recreated row cannot reuse it, which is exactly the distinction a slug lacks. Table names stay out of it for the original reason: they are rewritten as each rename commits, so hashing them would stop matching partway through the run the hash exists to protect.
5d2ef2f to
d02993e
Compare
The table path was moved out of its transaction and the column path was left behind. The observer reads through the adapter, which checks out its own connection, so introspecting from inside the transaction waits for a second checkout and hangs a pool sized to one — every discriminator rename, on the two dialects that pool. Both paths are now asserted by one test, because fixing one and leaving its sibling is what happened here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d02993edcc
ℹ️ 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: 3c7d60175b
ℹ️ 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".
pg_class.relname is unique per schema rather than per database, so an index query filtering on relname alone matched every schema holding that name and merged their indexes into one table's snapshot. The column query beside it was already scoped to public.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e1a40e677
ℹ️ 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".
…nly a sync's The signal handler was registered for every claim, so an interrupted migration released its lock while its work was still in flight — a signal does not stop the callback — letting a second process resume the same run against a database the first was still writing to. That is the failure the lock's no-expiry design exists to prevent, and the comment above it already said so. Recovery is now opt-in and taken only by schema syncs, where two overlapping syncs are the state that existed before this exclusion. An interrupted migration stays held for an operator, as designed.
|
@codex please review this PR |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 17 minutes. |
Every other test of this module talks to a stand-in, and a stand-in is what hid the fact that the lock could not run at all. This covers seed, claim, exclusion and release through the real adapter, with the lock table genuinely absent from the schema registry. Also refuses an unrecognised driver result in the mysql transaction reader rather than reporting it as no rows, and stops an interrupted release from surfacing as an unhandled rejection instead of letting the process stop.
|
@codex please review this PR |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 11 minutes. |
There was a problem hiding this comment.
💡 Codex Review
Model the migration lock table through Drizzle
On the first auto-sync for a database without this lock table, ensureLockRow sends this hand-built CREATE TABLE string through executeQuery. That introduces raw SQL database access in product code despite the repository's Drizzle-only invariant; define the lock table through the dialect schemas or another Drizzle-backed production DDL abstraction instead.
AGENTS.md reference: AGENTS.md:L83-L85
ℹ️ 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".
…applies next dev routes config edits through the reload path rather than the CLI watcher, so the schema-applying path most users are on had no guard at all: it builds field-group diffs and applies DDL, and its pre-cleanup issues UPDATE and DELETE, none of which can be reasoned about against half-renamed storage. The reload is abandoned rather than throwing, because a refused reload leaves the previous config in place while an exception here reaches the dev server and turns every later request into a 500. A refusal and a check that could not run are logged differently: the first is routine, the second would otherwise disable schema applies silently.
|
@codex please review this PR |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 59 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/nextly/src/domains/field-groups/migration/__tests__/session.test.ts (1)
134-137: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the
takeOverhelper’s purpose.Add a short comment explaining that this mutates the lock owner to simulate another process taking ownership for release-safety tests.
Suggested change
+ // Simulate another process taking ownership so release checks run against a changed claim. takeOver: (owner: string | null) => {🤖 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/migration/__tests__/session.test.ts` around lines 134 - 137, Add a concise comment above the takeOver helper explaining that it mutates the lock owner to simulate another process taking ownership for release-safety tests.Sources: Coding guidelines, Learnings
packages/nextly/src/domains/field-groups/migration/__tests__/state.test.ts (1)
705-716: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the plan guard fixtures to use
registryHash.
assertPlanUnchangedcomparesrecorded.registryHashvscurrent.registryHash, but this test still declares bothPLANandrefusalFromwithmanifestHash/planHash. As a result, the unchanged/resume test objects pass without a registry hash while the field-group drift test adds onlyregistryHashonto the wrong shape. Use{ manifestHash: "...", registryHash: "slugs-1" }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/field-groups/migration/__tests__/state.test.ts` around lines 705 - 716, Update the plan guard fixtures used by refusalFrom and related tests to replace planHash with registryHash, using the shape { manifestHash, registryHash } consistently for PLAN, current inputs, and unchanged/resume objects. Ensure the field-group drift fixture sets registryHash on that same shape so assertPlanUnchanged compares the intended values.
🧹 Nitpick comments (1)
packages/nextly/src/cli/commands/db-sync.ts (1)
281-293: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove this block to the
withMigrationExcludedcall it now describes.The guard is no longer a statement at this position, so the "Step 3.55" block documents nothing where it sits; the placement and watch-mode rationale belong beside the wrapper at Line 313.
🤖 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/db-sync.ts` around lines 281 - 293, Move the “Step 3.55” migration-exclusion comment from its current location to the withMigrationExcluded call around the sync execution. Keep the explanation of holding the exclusion across the sync and leaving watch mode outside adjacent to that wrapper, removing the misplaced standalone block.
🤖 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/domains/field-groups/migration/__tests__/session.test.ts`:
- Around line 387-405: The process.kill spy created in the “keeps a migration's
claim held when the process is interrupted” test must always be restored. Wrap
the withMigrationSession execution and assertions in a try/finally block,
placing kill.mockRestore() in finally so cleanup occurs even when setup, the
callback, or an assertion fails.
In `@packages/nextly/src/init/reload-config.ts`:
- Around line 968-976: Update the error-handling branch in runReload around
NextlyError.is(error) to use optional logger invocation for both warning and
error messages, matching the logger?. pattern used elsewhere in the function.
Preserve the existing messages and describeError(error) details while ensuring
an undefined logger cannot throw.
- Around line 953-961: Wrap the HMR reload schema-work sequence after
assertNoMigrationInFlight in withMigrationExcluded, passing the adapter/logger
context and mayCreateLock: false, and execute the diff, introspection,
PushSchemaPipeline DDL, and RealPreCleanupExecutor UPDATE/DELETE work inside its
callback. Preserve the existing migration check and ensure the exclusion covers
the entire reload operation through completion.
---
Outside diff comments:
In
`@packages/nextly/src/domains/field-groups/migration/__tests__/session.test.ts`:
- Around line 134-137: Add a concise comment above the takeOver helper
explaining that it mutates the lock owner to simulate another process taking
ownership for release-safety tests.
In `@packages/nextly/src/domains/field-groups/migration/__tests__/state.test.ts`:
- Around line 705-716: Update the plan guard fixtures used by refusalFrom and
related tests to replace planHash with registryHash, using the shape {
manifestHash, registryHash } consistently for PLAN, current inputs, and
unchanged/resume objects. Ensure the field-group drift fixture sets registryHash
on that same shape so assertPlanUnchanged compares the intended values.
---
Nitpick comments:
In `@packages/nextly/src/cli/commands/db-sync.ts`:
- Around line 281-293: Move the “Step 3.55” migration-exclusion comment from its
current location to the withMigrationExcluded call around the sync execution.
Keep the explanation of holding the exclusion across the sync and leaving watch
mode outside adjacent to that wrapper, removing the misplaced standalone block.
🪄 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: 0e5269d0-0993-41e3-9617-bdfdb209ee22
📒 Files selected for processing (12)
packages/adapter-mysql/src/index.tspackages/nextly/src/cli/commands/db-sync.tspackages/nextly/src/cli/commands/dev-watcher.tspackages/nextly/src/domains/field-groups/migration/__tests__/session.integration.test.tspackages/nextly/src/domains/field-groups/migration/__tests__/session.test.tspackages/nextly/src/domains/field-groups/migration/__tests__/state.test.tspackages/nextly/src/domains/field-groups/migration/__tests__/sync-guard.test.tspackages/nextly/src/domains/field-groups/migration/session.tspackages/nextly/src/domains/field-groups/migration/state.tspackages/nextly/src/domains/field-groups/migration/sync-guard.tspackages/nextly/src/init/__tests__/reload-config.test.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: 85b19636e6
ℹ️ 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".
…ust check it A migration starting between the check and the apply left the same window open the check was added to close. Holding is safe on this path specifically because reloads are already serialized in the process — one is kept in flight and the next is queued — so taking the lock cannot make a concurrent edit lose its turn. The reload body moved behind a wrapper rather than being re-indented, so the diff stays reviewable. A refused exclusion is told apart from a failure inside the reload, which still propagates as before, and the logger is optional at every new call site like the rest of the function. The lock double is now shared between the session and reload suites instead of each keeping its own, and the process.kill spies restore in finally so a failed assertion cannot leave the global mocked for later tests.
|
@codex please review this PR |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 43 minutes. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Slice B1-3c of task 011. Continues from #408 (reconciliation). Five commits, each independently verified; nothing calls the migration itself yet except the sync guard, which is live.
What this is
The migration could plan its renames and check that plan against the catalog. It could not yet run them. This makes each step executable and self-verifying, and closes the one gap that could destroy data before the migration is even wired up.
1. The recorded plan is what a resume executes
A resume used to rebuild its plan from registry rows and compare two hashes. That stops working the moment a rename updates its registry pointer: the rebuilt plan omits every row already renamed, so it has fewer entries, different step positions, and a different hash from the one the marker recorded — and the resume refuses the very plan it is resuming.
The plan is now persisted for both directions and read back. Identity moved with it: the stored plan is checked against a hash of itself, and "did the world move underneath this run" is answered by hashing the sorted slug set. Slugs, because table names are rewritten as the run progresses — a name-based comparison would refuse every resume past step one.
2. Each table rename carries its registry pointer
Renaming
comp_herotofg_herowhiledynamic_components.table_namestill sayscomp_heroleaves every row addressing a table that is gone, and the read path turns that into empty content rather than an error. The two now land in one transaction, andverifychecks both.Which name the pointer update targets is derived from the plan, not probed for: the registry renames last going up and first on a rollback, so the answer depends on position within that plan. Mid-run both names can be observable on a folding server, and reading it from the catalog would repeat a judgement reconciliation already owns.
Both halves are idempotent because MySQL commits DDL implicitly — a rename can land while the pointer update fails, so a resume applies only the half that is missing.
3.
db:syncrefuses while a migration is in flightThe highest-risk item here. Mid-run some tables carry pre-rename names and some post-rename, and
--remove-orphaneddeletes what it cannot account for. An unreadable marker refuses rather than reading as absent: absence means no migration ever ran, while an unreadable marker may still describe renamed objects.4. Index survival is checked by name
From the existing
introspectLiveSnapshot— no new adapter methods. By name rather than by count, because a count survives losing one index and gaining another, which is the exact shape of the SQLite index loss. An untracked index list staysundefinedrather than collapsing to[], so a caller cannot mistake "nothing was lost" for "nothing could be compared".Verification
pnpm build17/17 ·pnpm check-types19/19 ·pnpm lintexit 0 · fullpackages/nextlyrun 401, matching the baseline measured on main, with 0 branch-only failures.Every behavioural change has a test proven load-bearing by breaking the implementation and confirming the intended test failed with the count unchanged — 11 breaks across the five commits.
Worth a reviewer's attention
quoteIdent.Summary by CodeRabbit
New Features
Bug Fixes
Tests