feat(root): gate the field-group storage spelling to one catalog - #705
Conversation
|
Warning Review limit reached
Next review available in: 52 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change centralizes field-group discriminator key compatibility, updates collection, field-group, version, webhook, and schema paths, adds migration-focused tests, and enforces storage-format literal usage in CI. ChangesField-group storage migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ComponentInstance
participant FieldGroupTypeKey
participant CollectionQueryService
participant RestoreSnapshot
ComponentInstance->>FieldGroupTypeKey: readFieldGroupType(instance)
FieldGroupTypeKey-->>CollectionQueryService: resolved component type
FieldGroupTypeKey-->>RestoreSnapshot: resolved component type
RestoreSnapshot->>FieldGroupTypeKey: clearFieldGroupType(row)
RestoreSnapshot->>FieldGroupTypeKey: writeFieldGroupType(row, type)
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex please review this PR |
![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: a282705438
ℹ️ 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".
@nextlyhq/adapter-drizzle
@nextlyhq/adapter-mysql
@nextlyhq/adapter-postgres
@nextlyhq/adapter-sqlite
@nextlyhq/admin
@nextlyhq/admin-css
@nextlyhq/blocks-engine
@nextlyhq/blocks-react
@nextlyhq/builder
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: |
github-actions
Bot
added
scope: core
|
@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: 1908aad51c
ℹ️ 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: 4976cb2e63
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A catalog read of the wire key resolves correctly and still consults one spelling, so after the rename it reads a document written under the other one as untyped exactly as a hardcoded reader would. Route the reads, the key-set membership and the removals through the accessor, which tries every spelling, and keep the writes on the current one so the legacy set shrinks. Widen the gate to that property: the wire key is reached only through the accessor, and the column and table names are matched without requiring quotes, which ordinary property syntax does not use. Pin the outstanding admin sites individually so a new one fails instead of joining them.
The property is invisible before the rename: while the catalog still holds the legacy spelling, a single-key reader and a dual one behave identically, so the catalog is mocked to the post-rename state to separate them.
A scan reports on the forms it can see. Naming the ones it cannot -- a spelling assembled at runtime, the wholesale test exemption, the pinned admin sites, comment prose -- means the next reader inherits the limit instead of reading a green run as completeness.
The example showed one spelling of the discriminator path, which reads as the only accepted one and teaches it to anyone copying the query. Note the limit in the gate too: it filters comments, and a doc example is a comment it cannot tell from prose.
4976cb2 to
8ef5aa9
Compare
|
@codex please review this PR |
The discriminator path in a where clause comes from the caller, so it may name any spelling the key has carried. Matching one does not fail the filter, it reclassifies it as an ordinary field lookup and returns the wrong rows. Both where syntaxes decide this independently.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
.github/workflows/ci.yml (1)
103-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a short comment describing the gate scope.
Every other cheap gate step in this job carries a comment stating what it enforces. Lines 92-94 and 98-99 do this for the drizzle gates. This step has none. State that the gate bans raw field-group storage spellings outside the catalog, the accessor, the resolver, and the migration engine, and that
packages/adminsites are reported without failing.♻️ Proposed change
+ # Field-group storage-spelling gate: the stored table, column and JSON key + # names are read from `schemas/storage-format.ts`, never written out. Known + # `packages/admin` sites are reported without failing. Cheap check, fails fast. - name: Storage-spelling gate (field-group storage format) run: pnpm check:storage-format-literals🤖 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 @.github/workflows/ci.yml around lines 103 - 105, Add a concise comment immediately before the “Storage-spelling gate (field-group storage format)” step documenting that it bans raw field-group storage spellings outside the catalog, accessor, resolver, and migration engine, while reporting—but not failing—sites under packages/admin.scripts/check-storage-format-literals.cjs (3)
244-282: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the scanned extensions or state the limit in the header.
includedefaults to["*.ts", "*.tsx"]on line 149, and no call site overrides it. Source files with.mts,.cts,.js,.jsx, or.mjsextensions are never scanned. A hardcoded spelling in any of those files passes the gate.The header at lines 31-54 enumerates what a pass does not mean. Add the extension limit there, or widen the default include list.
♻️ Proposed change
- include = ["*.ts", "*.tsx"], + include = ["*.ts", "*.tsx", "*.mts", "*.cts", "*.js", "*.jsx", "*.mjs"],🤖 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 `@scripts/check-storage-format-literals.cjs` around lines 244 - 282, Update the scanner configuration around the default include list and its header documentation: either add .mts, .cts, .js, .jsx, and .mjs to the extensions scanned by the check, or explicitly document that those extensions are excluded. Ensure the behavior and documentation agree, without changing the existing literal checks.
147-176: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle grep exit status 2 for absent search paths.
pathsdefaults to["packages", "apps", "templates"]. If any of these directories does not exist in the repository root, GNU grep writesNo such file or directoryand exits 2. The catch block rethrows for any status other than 1, so the gate crashes with a stack trace instead of a verdict. The same applies to unreadable files.Filter the path list to directories that exist before calling grep.
♻️ Proposed change
+const fs = require("node:fs"); + function grep(label, pattern, opts = {}) { const { include = ["*.ts", "*.tsx"], paths = ["packages", "apps", "templates"], allowMatches = [], } = opts; + // Absent roots make grep exit 2, which is a crash rather than a verdict. + const searchPaths = paths.filter(p => fs.existsSync(path.join(ROOT, p))); + if (searchPaths.length === 0) { + throw new Error(`storage-spelling gate: no search paths exist (${paths.join(", ")})`); + } const args = [ "-rEn", pattern, - ...paths, + ...searchPaths,🤖 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 `@scripts/check-storage-format-literals.cjs` around lines 147 - 176, Update grep to filter the configured paths in opts.paths to existing directories under ROOT before constructing the arguments for execFileSync. Preserve valid paths and existing behavior for no matches, while preventing absent search paths from producing exit status 2 and crashing the check.
190-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the destructured
pathto avoid shadowing thenode:pathimport.Line 191 binds
pathinsidegrep(). That shadows the module-levelpathfromnode:pathfor the rest of the loop body. No current code callspath.*insidegrep(), so nothing breaks today. Any later use ofpath.joininside this function throws aTypeError. Thefs.existsSync(path.join(...))fix suggested for lines 147-176 is exactly that case.♻️ Proposed change
- for (const [path, texts] of Object.entries(PENDING_OCCURRENCES)) { + for (const [filePath, texts] of Object.entries(PENDING_OCCURRENCES)) { for (const text of texts) { - const key = `${path}::${text}`; + const key = `${filePath}::${text}`; remainingBudget.set(key, (remainingBudget.get(key) ?? 0) + 1); } }🤖 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 `@scripts/check-storage-format-literals.cjs` around lines 190 - 196, Rename the destructured `path` binding in the `PENDING_OCCURRENCES` loop to a distinct name, and update the corresponding key construction to use it, preserving the module-level `node:path` import for any `path.*` calls in the loop.
🤖 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/collections/services/collection-mutation-service.ts`:
- Line 98: Add a concise code-only comment immediately above the
readFieldGroupType import explaining that it supports reading both current and
legacy field-group discriminator keys for component password stripping.
In `@packages/nextly/src/domains/field-groups/storage/field-group-type-key.ts`:
- Around line 131-135: Update writeFieldGroupType to remove all supported
alternate discriminator keys from instance before assigning
currentFieldGroupTypeKey, preserving only the canonical key and provided type.
Add a regression test that initializes instance with an alternate supported key
and verifies it is removed after writing.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 103-105: Add a concise comment immediately before the
“Storage-spelling gate (field-group storage format)” step documenting that it
bans raw field-group storage spellings outside the catalog, accessor, resolver,
and migration engine, while reporting—but not failing—sites under
packages/admin.
In `@scripts/check-storage-format-literals.cjs`:
- Around line 244-282: Update the scanner configuration around the default
include list and its header documentation: either add .mts, .cts, .js, .jsx, and
.mjs to the extensions scanned by the check, or explicitly document that those
extensions are excluded. Ensure the behavior and documentation agree, without
changing the existing literal checks.
- Around line 147-176: Update grep to filter the configured paths in opts.paths
to existing directories under ROOT before constructing the arguments for
execFileSync. Preserve valid paths and existing behavior for no matches, while
preventing absent search paths from producing exit status 2 and crashing the
check.
- Around line 190-196: Rename the destructured `path` binding in the
`PENDING_OCCURRENCES` loop to a distinct name, and update the corresponding key
construction to use it, preserving the module-level `node:path` import for any
`path.*` calls in the loop.
🪄 Autofix
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: f00a531b-2879-42ba-96fe-b907b6cadcab
⛔ Files ignored due to path filters (1)
.changeset/storage-format-literal-gate.mdis excluded by!.changeset/**
📒 Files selected for processing (21)
.github/workflows/ci.ymlpackage.jsonpackages/nextly/src/domains/collections/query/query-operators.tspackages/nextly/src/domains/collections/services/collection-mutation-service.tspackages/nextly/src/domains/collections/services/collection-query-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/field-groups/services/field-group-utils.tspackages/nextly/src/domains/field-groups/storage/__tests__/field-group-type-key.post-flip.test.tspackages/nextly/src/domains/field-groups/storage/__tests__/field-group-type-key.test.tspackages/nextly/src/domains/field-groups/storage/field-group-type-key.tspackages/nextly/src/domains/schema/services/type-generator.tspackages/nextly/src/domains/singles/services/single-mutation-service.tspackages/nextly/src/domains/versions/__tests__/restore-snapshot.test.tspackages/nextly/src/domains/versions/__tests__/tag-component-types.post-flip.test.tspackages/nextly/src/domains/versions/diff/compute-diff.tspackages/nextly/src/domains/versions/restore-snapshot.tspackages/nextly/src/domains/versions/snapshot-references.tspackages/nextly/src/domains/versions/tag-component-types.tspackages/nextly/src/domains/webhooks/envelope.tsscripts/check-storage-format-literals.cjs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ef5aa9447
ℹ️ 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 |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex 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: ecefaa2324
ℹ️ 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".
1 participant
Why
The components → field-groups storage migration renames a JSON key inside stored content. Table and column names survive that because
storage/resolve-storage-names.tsreads the catalog and addresses whichever name is really there.🔴 A JSON key inside a row has no catalog. Nothing can observe it, so nothing can resolve it — and
schemas/storage-format.tswas being bypassed by 43 mentions of the raw spelling in product code, with nothing enforcing it.The consequence is not deletion. After a flip, a hardcoded reader looks for a key the migrated document no longer has and finds nothing: the instance is intact and untyped, which the editor cannot render, a diff cannot tag, and a
_componentTypefilter cannot match. To a live site that is indistinguishable from data loss.The write side is worse.
ComponentInput.tsx:202doesdefaultValues._componentType = ..., so a flip would leave the admin stamping the legacy spelling onto new documents behind a completed migration — divergence that grows for as long as the app runs.What this does
1. A dual-reading accessor (
domains/field-groups/storage/field-group-type-key.ts).A constant does not help a reader: inlining it still yields exactly one spelling. So the accessor tries both, in the order the migration itself uses, and writes only the current one. That pairing is what makes a partially rewritten database safe rather than merely survivable — every row still reads, and anything saved afterwards is written in the current spelling, so the legacy set only ever shrinks.
2. A gate (
scripts/check-storage-format-literals.cjs), wired intopackage.jsonand CI besidecheck-drizzle-v1-legacy.Installed first, before the fixes. Installed afterwards it would only record the current state; installed first it proves the binding is real.
3. Twelve
nextlysites routed through the accessor — including four that indexed the catalog correctly but still named the literal in a cast, which would go stale at the flip.Scope, stated rather than hidden
The five
packages/adminsites are not fixed here. Admin cannot reach the catalog —STORAGE_FORMATis exported from no surface it imports — so fixing them needs a public accessor export, which is an API decision rather than a mechanical edit.They are reported by every run as known-outstanding, not allowlisted:
An allowlist would make them disappear, and a gate whose output shrinks silently is how the original 43 accumulated.
Verification
The tests could not have worked by accident. The two spellings are the same string today, so every "falls back to the legacy key" assertion passes trivially against the real constants — a suite written against them would report coverage it does not have, right up until the flip made it matter. The target spelling is therefore mocked to something genuinely different, which is the only way to see the fallback work before the day it has to.
Proven by breaking, both halves:
The gate was checked as an instrument, not just used as one: with a literal planted it exits
1; clean it exits0. A gate that reports a failure and exits0blocks nothing.build,check-types,lint(by exit code),check-drizzle-v1-legacy, and the new gate: all0packages/nextlyunit run vs a freshly measured baseline at4e4309aec: 0 moved, 0 removed, 7 addedtrust-boundembed an absolute path in their test name, so any baseline diff taken across two worktrees reports them as moved. They are the same passing test under a different directory. Normalising the path out gives the 0/0/7 above. Worth fixing separately — it makes cross-worktree baselines noisy for everyone.What this does not do
It does not arm anything. The storage migration engine remains dark. This closes the largest of the blockers in front of it;
nextly migrateholding no exclusion, and field-group update still running DDL from its dispatcher, remain open and filed.Summary by CodeRabbit
Bug Fixes
Tests
Chores