refactor: Refactor reference extraction to use functional approach with refContext and provides instead of withRefBuilder
#702
Conversation
…plementation and implementing slots correctly
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 17f9245 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis PR refactors the reference extraction and schema-building system from a recursive, context-driven approach to a functional, pipeline-based approach using typed context slots. Core changes include introducing Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes This refactor is substantial and intricate, spanning the entire reference extraction pipeline, schema-builder infrastructure, and propagating across ~90 files with heterogeneous changes:
Areas requiring extra attention:
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
packages/project-builder-lib/src/tools/model-merger/model-merger.unit.test.ts (1)
777-790: GraphQL relation now using model name instead of ID is consistent with ref refactorSwitching
modelRefto the literal'Author'here aligns the desired GraphQL/Post relation with the existing config (which already usesauthorModel.name) and with the broader move to stable, name-based references in the new ref/slot system. Functionally this looks correct.If you want to avoid duplicating the
'Author'string, you could optionally pull the model name into a sharedconstand reuse it in bothauthorModeland this relation, but that's a nice‑to‑have, not required.packages/project-builder-lib/src/schema/creator/schema-creator.ts (1)
56-72: Add explicit return type annotation.Per coding guidelines, top-level functions should include return types. While TypeScript can infer the return type, an explicit annotation improves readability and catches potential type mismatches earlier.
export function definitionSchemaWithSlots< TSlotDef extends RefContextSlotDefinition, T extends z.ZodType, >( slotDefinition: TSlotDef, creator: ( ctx: DefinitionSchemaParserContext, slots: RefContextSlotMap<TSlotDef>, ) => T, -): DefinitionSchemaCreatorWithSlots<T, TSlotDef> { +): DefinitionSchemaCreatorWithSlots<T, TSlotDef> { const creatorWithSlots: DefinitionSchemaCreatorWithSlots<T, TSlotDef> = ( context, slots, ) => creator(context, slots); creatorWithSlots.slotDefinition = slotDefinition; return creatorWithSlots; }The return type is already present, so this is actually correct. The documentation example is clear and demonstrates the slot-based pattern well.
packages/project-builder-lib/src/schema/apps/backend/index.ts (1)
10-22: The creator function ignoresctxandslotsparameters.Based on
definitionSchemaWithSlotssignature inschema-creator.ts, the creator receives(ctx: DefinitionSchemaParserContext, slots: RefContextSlotMap<TSlotDef>). The current implementation uses() =>which discards both parameters. Ifctxandslotsare intentionally unused, consider using(_ctx, _slots) =>to make this explicit and avoid potential lint warnings about unused parameters.export const createBackendAppSchema = definitionSchemaWithSlots( { appSlot: appEntityType }, - () => + (_ctx, _slots) => z.object({ ...baseAppValidators, type: z.literal('backend'), enableStripe: z.boolean().optional(), enableBullQueue: z.boolean().optional(), enablePostmark: z.boolean().optional(), enableSubscriptions: z.boolean().optional(), enableAxios: z.boolean().optional(), }), );packages/project-builder-lib/src/references/collect-refs.ts (2)
32-50: Consider using explicit type annotation instead of type assertion.The type assertion
as CollectedRefson line 50 could be replaced with an explicit type annotation on the variable declaration for better type safety.- const collected = { + const collected: CollectedRefs = { entities: [], references: [], slots: [], - } as CollectedRefs; + };
89-100: Redundant type check for object keys.
Object.entries()always returns string keys in JavaScript, so the checktypeof key !== 'string'on line 90 will never be true. This can be safely removed.for (const [key, childValue] of Object.entries(value)) { - if (typeof key !== 'string') continue; const childCollected = collectRefAnnotationsRecursive( [...pathPrefix, key], childValue, );packages/project-builder-lib/src/references/resolve-slots.ts (1)
100-105: Consider usingpush()for minor performance improvement.The current spread pattern creates a new array on each registration. For schemas with many slots of the same type, using
push()would be more efficient.- const existingSlots = resolvedSlotsByType.get(slotId) ?? []; - resolvedSlotsByType.set(slotId, [ - ...existingSlots, - { resolvedPath, path: nearestAncestorSlot.path }, - ]); + let existingSlots = resolvedSlotsByType.get(slotId); + if (!existingSlots) { + existingSlots = []; + resolvedSlotsByType.set(slotId, existingSlots); + } + existingSlots.push({ resolvedPath, path: nearestAncestorSlot.path });packages/project-builder-lib/src/schema/apps/web/admin/sections/crud-columns/types.ts (1)
3-5: Consider extracting common slot interface pattern.Both this file and
transformers/types.tsdefine nearly identical interfaces and types:
ModelTransformerSlots/AdminCrudColumnSlotsModelTransformerSchemaCreator/AdminCrudColumnSchemaCreatorIf more slot-based schema creators are added, consider extracting a generic base pattern to reduce duplication.
Example of a potential shared pattern:
// In a shared types file export interface ModelSlotContainer { modelSlot: RefContextSlot<typeof modelEntityType>; } export type SlotAwareSchemaCreator< TSlots, TSchema extends z.ZodType = z.ZodType, > = (ctx: DefinitionSchemaParserContext, slots: TSlots) => TSchema;Also applies to: 31-41
packages/project-builder-lib/src/references/extract-definition-refs.ts (1)
84-91: Minor: Comment says "Step 4" but this is actually Step 5.The JSDoc flow documentation mentions 4 steps, but the duplicate ID validation is effectively a 5th step. Consider updating the comment for consistency.
- // Step 4: Validate no duplicate IDs + // Step 5: Validate no duplicate IDspackages/project-builder-lib/src/schema/apps/web/admin/sections/crud-form/built-in-input.ts (1)
25-38: Consider whether unusedadminSectionSlotis intentional.The slot definition includes
adminSectionSlot, but onlymodelSlotis destructured and used. The same pattern appears increateAdminCrudForeignInputSchemaandcreateAdminCrudEnumInputSchema. This may be intentional for API consistency or future extensibility, but if these schemas will never needadminSectionSlot, consider removing it to keep the API minimal.If the unused slot is intentional for consistency, please confirm. Otherwise:
-export const createAdminCrudTextInputSchema = definitionSchemaWithSlots( - { modelSlot: modelEntityType, adminSectionSlot: adminSectionEntityType }, - (ctx, { modelSlot }) => +export const createAdminCrudTextInputSchema = definitionSchemaWithSlots( + { modelSlot: modelEntityType }, + (ctx, { modelSlot }) =>packages/project-builder-lib/src/references/extend-parser-context-with-refs.ts (1)
150-163: Consider improving error handling for missing name resolver.When
getNameResolveris not provided and thenamefield is missing, the function returns'invalid'as the nameResolver while still adding a validation issue. This means downstream code receives an entity annotation with an'invalid'nameResolver, which could cause confusing errors later.Consider either:
- Returning early after adding the issue (like the id validation block above)
- Using a more descriptive sentinel value or structured error
if (!('name' in value) || typeof value.name !== 'string') { ctx.addIssue({ code: 'custom', message: `Unable to find string name field in entity ${entity.type.name}`, input: value, }); - return 'invalid'; + return value; // Return early, similar to id validation } return value.name;Note: This would require adjusting the subsequent logic to handle the early return case.
kingston
deleted the
kingston/eng-929-ref-context-slots-reference-system-redesign
branch
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.