fix(nextly): create the table on every field-group create transport - #596
Conversation
…ma service Runtime registration and companion provisioning were private functions inside the components dispatcher, which is why the two other create transports could not reach the schema half and shipped a registry row describing a table that was never created. Both bodies move unchanged. Verified byte-identical against main; the only difference is the depth of three dynamic-import paths, which the new location changes.
… row The REST route wrote a registry row and made no table, so it answered 201 for a field group whose comp_<slug> did not exist and every later read and write to it failed against the database. FieldGroupMetadataService plans the DDL, applies it without throwing, and writes the row carrying the outcome the apply reached. It is registered in DI rather than built per request so one wrapper governs every caller, which is what lets a migration lock enclose both halves. The route now reports the status instead of answering an unqualified success for a create whose table was never made.
nextly.fieldGroups.create() and the mounted POST /api/field-groups route wrote a registry row and no table, then answered success. The registry described a comp_<slug> that did not exist and every read and write to that field group failed against the database. All three transports now go through FieldGroupMetadataService, so the table change and the registry write are one operation wherever a create comes from, and a create whose DDL failed says so instead of answering an unqualified success.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex please review this PR |
|
@coderabbitai full review |
|
@greptileai review |
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 36 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 ignored due to path filters (1)
📒 Files selected for processing (10)
📝 WalkthroughWalkthroughField-group creation now uses ChangesField-group provisioning
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant API
participant ComponentDispatcher
participant FieldGroupMetadataService
participant DatabaseAdapter
participant FieldGroupRegistryService
API->>FieldGroupMetadataService: createFieldGroup(input)
ComponentDispatcher->>FieldGroupMetadataService: createFieldGroup(component metadata)
FieldGroupMetadataService->>DatabaseAdapter: apply field-group DDL
DatabaseAdapter-->>FieldGroupMetadataService: provisioning status
FieldGroupMetadataService->>FieldGroupRegistryService: persist metadata record
FieldGroupMetadataService-->>API: record and migrationStatus
FieldGroupMetadataService-->>ComponentDispatcher: record and migrationStatus
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
@nextlyhq/blocks-react
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: |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nextly/src/dispatcher/handlers/component-dispatcher.ts (1)
244-249: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDistinguish
failedfrompendingin the response message.
createFieldGroupreturns three statuses.pendingmeans no adapter was registered and the DDL was never run.failedmeans the DDL threw, the table was absent after the migration, or companion provisioning failed. The current message tells the admin to run migrations in both cases. Forfailed, running migrations is not the corrective action, and the message hides a real error.♻️ Proposed message split
- const message = - migrationStatus === "applied" - ? `Component "${b.slug}" created and table applied!` - : `Component "${b.slug}" created. Run migrations to apply the table.`; + // Each status has its own corrective action: `pending` waits for a migration run, while + // `failed` means the DDL was attempted and did not land, so the server logs hold the cause. + const message = + migrationStatus === "applied" + ? `Component "${b.slug}" created and table applied!` + : migrationStatus === "failed" + ? `Component "${b.slug}" created, but its table could not be provisioned. Check the server logs and retry.` + : `Component "${b.slug}" created. Run migrations to apply the table.`;🤖 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/dispatcher/handlers/component-dispatcher.ts` around lines 244 - 249, Update the message selection near the migrationStatus check to handle all three statuses returned by createFieldGroup: retain the applied success message, keep the run-migrations guidance only for pending, and add a distinct failure message for failed that indicates table or companion provisioning failed without directing the admin to run migrations.
🧹 Nitpick comments (9)
packages/nextly/src/api/__tests__/field-group-route-delegates.test.ts (1)
101-107: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the status code in the failed-provisioning test.
expect(body.message).not.toBe("Field group created.")passes for any response, including an error response produced before the route reachescreateFieldGroup.requireBuilderEnabled("create-component")is not mocked in this file, so that path is reachable. Add a status assertion to pin the documented behavior: a failed provisioning still returns 201 with the created record.💚 Proposed addition
const body = (await response.json()) as { message?: string }; + expect(response.status).toBe(201); expect(body.message).not.toBe("Field group created."); expect(body.message).toContain("could not be provisioned");🤖 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/api/__tests__/field-group-route-delegates.test.ts` around lines 101 - 107, Update the failed-provisioning test around the response body assertion to also verify that the response status is 201, preserving the existing message checks and confirming the documented successful creation response when provisioning fails.packages/nextly/src/di/register.ts (1)
338-338: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the one-line doc the sibling entry carries.
singleMetadataServiceon line 337 documents that it owns the table change together with the registry write. GivefieldGroupMetadataServicethe equivalent line so the two parallel services read the same way.♻️ Proposed change
+ /** Owns a Field Group's table change together with the registry write that records it. */ fieldGroupMetadataService: FieldGroupMetadataService;🤖 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/di/register.ts` at line 338, Add the same one-line ownership documentation used by singleMetadataService immediately above fieldGroupMetadataService, stating that it owns the table change together with the registry write.packages/nextly/src/domains/field-groups/services/field-group-metadata-service.ts (3)
166-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
describeErrorfor these operator-facing logs.Both catch blocks in this method (lines 166-173 and 216-223) format the error with
error.message. That drops the cause chain, which is the part that identifies a driver failure. This repository usesdescribeError(error)for operator-facing logs and diagnostics. Neither message is used as a control-flow predicate, sodescribeErroris the right helper here.Based on learnings that
describeError(error)should be used for operator-facing logs and diagnostics, whileimmediateMessage(error)is reserved for message-based control-flow predicates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nextly/src/domains/field-groups/services/field-group-metadata-service.ts` around lines 166 - 173, Update both catch blocks in the field-group metadata service method to use the repository’s describeError(error) helper when formatting operator-facing logger.error messages, replacing the current error.message/String formatting while preserving each existing log context and return behavior.Source: Learnings
158-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the double assertion with a real type relation.
input.fields as unknown as FieldDefinition[]erases the difference betweenFieldConfig[]andFieldDefinition[]instead of resolving it. If the two shapes are compatible, widen thereconcileComponentCompanionparameter or declare the relation once in a shared type. If they are not compatible, the cast hides a real mismatch that reaches the companion DDL planner.As per coding guidelines: "Do not use
as any,@ts-expect-error, or eslint-disable directives to silence errors; fix the cause with real types, guards, or generics."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nextly/src/domains/field-groups/services/field-group-metadata-service.ts` around lines 158 - 159, Replace the double assertion in the metadata service with a real type relationship: inspect FieldConfig and FieldDefinition and either widen reconcileComponentCompanion’s parameter or define a shared compatible type, preserving type safety through the companion DDL planner. If the shapes are incompatible, add the necessary conversion or validation instead of casting.Source: Coding guidelines
4-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComments across four files document the previous defect instead of the current behavior. Each of these blocks explains what the old code did wrong and how this PR fixed it. Repo convention keeps comments on current behavior and non-obvious rationale, and excludes historical remediation. Keep the parts that state a present-tense contract, such as the atomicity and ordering guarantees.
packages/nextly/src/domains/field-groups/services/field-group-metadata-service.ts#L4-L13: remove the "Why this exists" narrative about the three transports and the dispatcher-private helper; keep the atomicity and ordering sections at lines 15-30.packages/nextly/src/api/field-groups.ts#L45-L51: drop the sentences describing what the route "used to" do; keep the statement that the service owns the table and the registry row together.packages/nextly/src/api/__tests__/field-group-route-delegates.test.ts#L1-L14: reduce the header to what the file verifies, which is delegation, and to the pointer at the integration test that verifies provisioning.packages/nextly/src/di/registrations/register-components.ts#L36-L40: remove the closing sentence about the transports being "allowed to disagree"; state the present rationale for the singleton registration.Based on learnings that source-file comments should document behavior and rationale for non-obvious logic only, and should avoid references to tasks, plans, conversations, review findings, or historical remediation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nextly/src/domains/field-groups/services/field-group-metadata-service.ts` around lines 4 - 13, Remove historical remediation commentary while preserving current behavior and rationale: in packages/nextly/src/domains/field-groups/services/field-group-metadata-service.ts lines 4-13, delete the “Why this exists” transport/dispatcher narrative and retain the atomicity and ordering sections; in packages/nextly/src/api/field-groups.ts lines 45-51, remove “used to” behavior and retain that the service owns table and registry-row creation; in packages/nextly/src/api/__tests__/field-group-route-delegates.test.ts lines 1-14, keep only the delegation purpose and integration-test pointer; in packages/nextly/src/di/registrations/register-components.ts lines 36-40, remove the transports-disagreement closing sentence and state the current rationale for singleton registration.Sources: Coding guidelines, Learnings
packages/nextly/src/domains/field-groups/services/field-group-table-provisioning.ts (1)
11-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueA domain module now depends on the dispatcher layer.
This file moved into
domains/field-groups/services/but still importsgetConfigFromDIandgetSchemaRegistryFromDIfrom../../../dispatcher/helpers/di. The dependency now points from the domain layer into the transport layer, which is the reverse of the direction the rest of this stack uses. Consider moving those two DI accessors to a shared location so the domain module does not reach intodispatcher/.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/nextly/src/domains/field-groups/services/field-group-table-provisioning.ts` around lines 11 - 14, Move getConfigFromDI and getSchemaRegistryFromDI out of dispatcher/helpers/di into an appropriate shared infrastructure or DI module, then update field-group table provisioning and existing consumers to import them from that shared location. Preserve both accessors’ behavior while removing the domain module’s dependency on dispatcher/.packages/nextly/src/direct-api/__tests__/field-groups-table-name.test.ts (1)
29-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNarrow the registry double instead of asserting through
unknown.
registry as unknown as FieldGroupRegistryServicehides which members the service actually needs. Type the double against the surface it must satisfy, so a future constructor dependency inFieldGroupMetadataServicefails at compile time rather than at runtime.♻️ Proposed change
- const registry = { registerComponent }; + const registry: Pick<FieldGroupRegistryService, "registerComponent"> = { + registerComponent, + }; const ctx = { fieldGroupRegistryService: registry, fieldGroupMetadataService: new FieldGroupMetadataService( - registry as unknown as FieldGroupRegistryService, + registry as FieldGroupRegistryService, logger ),As per coding guidelines: "Do not use
as any,@ts-expect-error, or eslint-disable directives to silence errors; fix the cause with real types, guards, or generics."🤖 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/direct-api/__tests__/field-groups-table-name.test.ts` around lines 29 - 38, Update the registry double used to construct FieldGroupMetadataService so it is typed against the minimal FieldGroupRegistryService surface required by that constructor, removing the unknown-based double assertion. Preserve the existing registerComponent behavior while ensuring future required registry members produce compile-time errors.Source: Coding guidelines
packages/nextly/src/api/field-groups.ts (1)
225-233: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reporting the provisioning failure through the
warningschannel.
respondMutationalready carries awarningsarray for exactly this case: a durable row plus a side-effect phase that failed. Encoding the failure only intomessagemeans a client must string-match to detect it. The record also carriesmigrationStatus, so the information is available, but a warning entry would make the failure machine-readable and consistent with the other mutation routes.🤖 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/api/field-groups.ts` around lines 225 - 233, Update the field-group creation response around respondMutation so a non-"applied" migrationStatus also adds a machine-readable warning through its warnings option, while retaining the existing message and 201 status. Keep the warning absent for successfully applied migrations and follow the warning structure used by other mutation routes.packages/nextly/src/dispatcher/handlers/component-dispatcher.ts (1)
216-227: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComments across this PR explain the previous defect instead of the current code. Each new comment below describes what the old implementation did wrong and how this PR repairs it. Repository guidelines and prior learnings require comments that state current behavior and the rationale for non-obvious logic, with no reference to past findings or remediation. Apply the same rule at each site.
packages/nextly/src/dispatcher/handlers/component-dispatcher.ts#L216-L227: state that one service owns both the DDL and the registry write, and drop the sentence about this handler previously holding the DDL.packages/nextly/src/dispatcher/handlers/component-dispatcher.ts#L133-L135: state where the helpers live and why every transport uses them, and drop the "now live" comparison with their previous location.packages/nextly/src/direct-api/namespaces/field-groups.ts#L176-L178: state that the service writes the row with the provisioning outcome, and drop the sentence about this path previously answering success.packages/nextly/src/domains/field-groups/__tests__/field-group-create-transports.integration.test.ts#L1-L29: keep the invariant under test, the reason the REST transport is covered elsewhere, and the per-dialect skip rule; drop "The gap this closes" and the coverage-history narrative.Based on learnings: "source-file comments should document the code's behavior and the rationale for non-obvious logic only. Avoid comments that reference tasks/plans, conversations, review findings, or historical remediation." As per coding guidelines: "Every code change must include a comment explaining what the code does and why; comments must not reference tasks, plans, conversations, or review findings."
🤖 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/dispatcher/handlers/component-dispatcher.ts` around lines 216 - 227, Update the comments at packages/nextly/src/dispatcher/handlers/component-dispatcher.ts#L216-L227 to describe that one service owns both DDL and registry writes, removing historical wording; at packages/nextly/src/dispatcher/handlers/component-dispatcher.ts#L133-L135, explain where the helpers live and why every transport uses them without comparing their former location; at packages/nextly/src/direct-api/namespaces/field-groups.ts#L176-L178, state that the service writes the row with the provisioning outcome and remove prior-path history; and at packages/nextly/src/domains/field-groups/__tests__/field-group-create-transports.integration.test.ts#L1-L29, retain the tested invariant, separate REST coverage rationale, and per-dialect skip rule while removing coverage-gap and history narrative.Sources: Coding guidelines, 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/di/registrations/register-components.ts`:
- Around line 36-49: Add the migration lock around the complete create flow in
FieldGroupMetadataService.createFieldGroup, covering both field-group table
changes and the fieldGroupRegistryService.registerComponent write, so concurrent
creates for the same slug cannot overlap. Ensure the singleton factory in
register-components.ts still supplies the shared service instance, and update
its explanatory comment only if the lock is implemented elsewhere.
---
Outside diff comments:
In `@packages/nextly/src/dispatcher/handlers/component-dispatcher.ts`:
- Around line 244-249: Update the message selection near the migrationStatus
check to handle all three statuses returned by createFieldGroup: retain the
applied success message, keep the run-migrations guidance only for pending, and
add a distinct failure message for failed that indicates table or companion
provisioning failed without directing the admin to run migrations.
---
Nitpick comments:
In `@packages/nextly/src/api/__tests__/field-group-route-delegates.test.ts`:
- Around line 101-107: Update the failed-provisioning test around the response
body assertion to also verify that the response status is 201, preserving the
existing message checks and confirming the documented successful creation
response when provisioning fails.
In `@packages/nextly/src/api/field-groups.ts`:
- Around line 225-233: Update the field-group creation response around
respondMutation so a non-"applied" migrationStatus also adds a machine-readable
warning through its warnings option, while retaining the existing message and
201 status. Keep the warning absent for successfully applied migrations and
follow the warning structure used by other mutation routes.
In `@packages/nextly/src/di/register.ts`:
- Line 338: Add the same one-line ownership documentation used by
singleMetadataService immediately above fieldGroupMetadataService, stating that
it owns the table change together with the registry write.
In `@packages/nextly/src/direct-api/__tests__/field-groups-table-name.test.ts`:
- Around line 29-38: Update the registry double used to construct
FieldGroupMetadataService so it is typed against the minimal
FieldGroupRegistryService surface required by that constructor, removing the
unknown-based double assertion. Preserve the existing registerComponent behavior
while ensuring future required registry members produce compile-time errors.
In `@packages/nextly/src/dispatcher/handlers/component-dispatcher.ts`:
- Around line 216-227: Update the comments at
packages/nextly/src/dispatcher/handlers/component-dispatcher.ts#L216-L227 to
describe that one service owns both DDL and registry writes, removing historical
wording; at
packages/nextly/src/dispatcher/handlers/component-dispatcher.ts#L133-L135,
explain where the helpers live and why every transport uses them without
comparing their former location; at
packages/nextly/src/direct-api/namespaces/field-groups.ts#L176-L178, state that
the service writes the row with the provisioning outcome and remove prior-path
history; and at
packages/nextly/src/domains/field-groups/__tests__/field-group-create-transports.integration.test.ts#L1-L29,
retain the tested invariant, separate REST coverage rationale, and per-dialect
skip rule while removing coverage-gap and history narrative.
In
`@packages/nextly/src/domains/field-groups/services/field-group-metadata-service.ts`:
- Around line 166-173: Update both catch blocks in the field-group metadata
service method to use the repository’s describeError(error) helper when
formatting operator-facing logger.error messages, replacing the current
error.message/String formatting while preserving each existing log context and
return behavior.
- Around line 158-159: Replace the double assertion in the metadata service with
a real type relationship: inspect FieldConfig and FieldDefinition and either
widen reconcileComponentCompanion’s parameter or define a shared compatible
type, preserving type safety through the companion DDL planner. If the shapes
are incompatible, add the necessary conversion or validation instead of casting.
- Around line 4-13: Remove historical remediation commentary while preserving
current behavior and rationale: in
packages/nextly/src/domains/field-groups/services/field-group-metadata-service.ts
lines 4-13, delete the “Why this exists” transport/dispatcher narrative and
retain the atomicity and ordering sections; in
packages/nextly/src/api/field-groups.ts lines 45-51, remove “used to” behavior
and retain that the service owns table and registry-row creation; in
packages/nextly/src/api/__tests__/field-group-route-delegates.test.ts lines
1-14, keep only the delegation purpose and integration-test pointer; in
packages/nextly/src/di/registrations/register-components.ts lines 36-40, remove
the transports-disagreement closing sentence and state the current rationale for
singleton registration.
In
`@packages/nextly/src/domains/field-groups/services/field-group-table-provisioning.ts`:
- Around line 11-14: Move getConfigFromDI and getSchemaRegistryFromDI out of
dispatcher/helpers/di into an appropriate shared infrastructure or DI module,
then update field-group table provisioning and existing consumers to import them
from that shared location. Preserve both accessors’ behavior while removing the
domain module’s dependency on dispatcher/.
🪄 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: b18814c4-773c-41f4-b97f-e9b8f4df7b97
⛔ Files ignored due to path filters (1)
.changeset/field-group-transports-provision.mdis excluded by!.changeset/**
📒 Files selected for processing (15)
packages/nextly/src/api/__tests__/field-group-route-delegates.test.tspackages/nextly/src/api/field-groups.tspackages/nextly/src/di/register.tspackages/nextly/src/di/registrations/register-components.tspackages/nextly/src/direct-api/__tests__/field-groups-table-name.test.tspackages/nextly/src/direct-api/namespaces/context.tspackages/nextly/src/direct-api/namespaces/field-groups.tspackages/nextly/src/direct-api/nextly.tspackages/nextly/src/dispatcher/handlers/__tests__/component-dispatcher-ddl.test.tspackages/nextly/src/dispatcher/handlers/__tests__/component-dispatcher-shapes.test.tspackages/nextly/src/dispatcher/handlers/component-dispatcher.tspackages/nextly/src/dispatcher/helpers/di.tspackages/nextly/src/domains/field-groups/__tests__/field-group-create-transports.integration.test.tspackages/nextly/src/domains/field-groups/services/field-group-metadata-service.tspackages/nextly/src/domains/field-groups/services/field-group-table-provisioning.ts
|
@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: be3117eda0
ℹ️ 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".
…und the slug The table-name conflict check lived in the dispatcher, so the Direct API and the mounted route reached the DDL without it: a slug normalising onto a table another field group owns rebound that field group's storage to the new field list before the registry rejected anything. It now runs inside the service that owns the DDL, and the dispatcher's copy is gone. The post-migration tableExists probe sat outside the catch, so a transient query failure rejected the apply and skipped the registry write, orphaning the table it had just made. It now shares the statements' catch and records failed. The route's slug bound was 255 while the rest of the product validates 50; a longer slug becomes an identifier PostgreSQL truncates and MySQL rejects. The bound is now named once in base-validator and used by both.
|
@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: 27369a75cd
ℹ️ 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".
… written Two creates whose slugs normalise to one table can both pass the ownership check, because a read cannot exclude a write that has not happened yet. The registry's table_name unique index is what separates them, and that only helps if nothing irreversible has happened first. Binding ran before the insert, so the request that loses rebound the shared table to its own field list and only then failed, leaving the winner reading through a schema that does not describe it until the process restarts. The bind now runs after the insert that would reject it.
|
@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: 2855850a8a
ℹ️ 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: 2855850a8a
ℹ️ 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".
…nerates The 50-character bound measured the slug. The longest thing the database sees is idx_ + comp_ + slug + _parent, sixteen characters longer, so a 50-character slug produced a 66-character index name: MySQL rejects past 64, leaving the table created, the index creation failed, and the field group recorded failed with an unbound table. The bound is now derived from the same constants the names are built from, so changing a prefix moves it rather than leaving a stale number. The budget is PostgreSQL's 63 rather than MySQL's 64 because it is tighter and its failure is worse: PostgreSQL truncates an over-long identifier instead of rejecting it.
|
@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: ab8b2d4262
ℹ️ 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".
…sport A slug bound cannot make this safe on its own. A field's index is named idx_<tableName>_<columnName>, so the longest identifier depends on the slug AND the longest indexed field name — two independent inputs, neither constraining the other. A slug at its own bound paired with authorId still generates a 66-character index. The check is therefore over the names themselves, enumerated by the service that builds them, and it runs before any DDL. Without it the table and its parent index are created, the field index fails, and the caller receives a record whose migration is recorded failed: a field group that exists and cannot be queried, from a request that returned a success shape. It lives in the metadata service rather than in a transport for the same reason the ownership check does — the mounted route bounded its slug and the other two transports did not.
|
@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: 6b2a655347
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The previous check walked the field list again to work out which names a create would generate. That was a second transcription of rules the renderer already applies, and it got three of them wrong: it missed unique indexes, which are named uq_ and need no index flag; it missed the column names themselves, which are identifiers too; and it counted an index for localized fields that the renderer never emits, refusing valid creates over a name that does not exist. It now scans the rendered statements. Every identifier this service emits is quoted, and no dialect uses that character for string literals, so the quoted tokens are identifiers and nothing else. A check over the output cannot drift from it. The failure message no longer tells callers to retry. A failed create keeps its registry row, and that row holds the slug, so a retry is refused as a duplicate through either transport; it now says to delete the field group first.
|
@codex please review this PR |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
The defect (P1, live, reachable through the public API)
A field group is creatable through three transports. Only one made the table.
component-dispatcher.tsapi/field-groups.tsPOSTdirect-api/namespaces/field-groups.tscreateBoth of the last two answered success. The route returned
201 Field group created.; the Direct API resolved normally. The registry then described acomp_<slug>table that did not exist, and every read and write to that field group failed against the database.This is not a private path:
nextly.fieldGroups.create()is the public programmatic surface, andapi/field-groups.tsis exported for user apps to mount. Both routes already callrequireBuilderEnabled("create-component"), so they were understood to BE schema operations. They just did not perform the schema half.Why it happened, and why the fix is a service
The code that provisions the table was private to the dispatcher —
registerComponentRuntimeSchemaandreconcileComponentCompanionwere file-local functions. Nothing else could reach them, so the other transports could not have done the right thing even by trying. Copying the DDL block into them would have entrenched exactly the split this closes.Three commits, each independently checkable:
fe6dda5a6— the two helpers move beside the field-group schema service. Proven a pure move: both bodies diffed byte-identical against main, the only difference being the depth of three dynamic-import paths.0604a56fd—FieldGroupMetadataService: plan the DDL, apply it without throwing, write the row carrying the outcome. Registered in DI rather than built per request, so one wrapper governs every caller — which is what lets the migration lock (B1-9b) enclose both halves.be3117eda— all three transports routed through it, plus the tests.The input type is
Omit<DynamicFieldGroupInsert, "migrationStatus">— the registry's own insert type, never a hand-listed subset. That is not stylistic: the singles equivalent listed its fields by hand and silently dropped three of them, and nothing in the types or the tests caught it.The coverage that was missing
Each transport was individually green and the product was broken, because the coverage was per transport rather than per outcome: the dispatcher had tests, and the other two were tested for their response shape and their table-name derivation — neither of which touches a database.
field-group-create-transports.integration.test.tsasserts the outcome against a real database: the physical table exists, per transport, per dialect.🔴 The REST route is proved a level up, and the test says so rather than leaving it implicit. It is permission-gated and the harness cannot authenticate a request, so proving it there would mean faking a session and testing the fake. Instead the split is explicit: the integration suite proves the shared service provisions the table on live databases through the two transports the harness can reach, and
field-group-route-delegates.test.tsproves the route reaches that service rather than writing the row itself. Neither half stands alone, and the file comment says so.That delegation test also asserts the negative:
registerComponentmust not be called. The defect was never that the row went unwritten — it was that the row was written alone, so a route calling both would satisfy a positive-only assertion.Verification
pnpm build·pnpm check-types --force·pnpm lint(exit code) ·node scripts/check-drizzle-v1-legacy.cjs— clean6ca3f4506. Zero new.Four existing suites needed rewiring. Each got the real service over the same doubles rather than a stub, so they keep describing the code instead of describing the stub.
One deliberate omission: the dispatcher keeps its original toast wording rather than adopting the routes' new message. Changing admin copy is not part of closing this P1.
Summary by CodeRabbit
New Features
Bug Fixes