refactor: Refactor sync/save flow of project definition #435
Conversation
ENG-469 When syncing and saving, recently modified error shows up
If you sync and then save, we show a recently modified error toast |
🦋 Changeset detectedLatest commit: e0a068f The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 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 |
WalkthroughThe changes span multiple packages and affect dependency updates, type definitions, method signatures, and file structures. Notable updates include dependency patches, migration from legacy payload types to new ResolvedZodRefPayload, and removal of deprecated classes (e.g. ParsedProjectDefinition). API context and router implementations have been refactored in the server package, while the web client sees extensive updates in component naming, form handling, and state management (using new saveDefinitionWithFeedback variants). New utilities and unit tests have been added, and several error handling mechanisms (including custom error types) have been introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ContextBuilder as createContextBuilder
participant API as Service Lookup
Client->>ContextBuilder: Request context (req, res)
ContextBuilder->>ContextBuilder: Build context with BaseplateApiContext
ContextBuilder->>API: getApi(projectId)
API-->>ContextBuilder: Return service instance or undefined
alt Service found
ContextBuilder->>Client: Return context with service instance
else Service not found
ContextBuilder->>Client: Throw TRPCError("NOT_FOUND")
end
sequenceDiagram
participant User
participant Provider as ProjectDefinitionProvider
participant Manager as ProjectDefinitionFileManager
participant API as Remote Service
User->>Provider: Submit updated project definition
Provider->>Manager: Upload new definition with old hash
Manager->>API: Call uploadProjectDefinition(id, newContents, oldHash)
API-->>Manager: Return updated file payload
Manager-->>Provider: New project definition payload
Provider->>User: Update UI and show success message
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (1)
packages/project-builder-web/src/pages/plugins/home.page.tsx (1)
77-79:⚠️ Potential issueFix incorrect array usage in plugin rendering.
The plugin rendering sections are using the full
pluginsarray instead of the filtered arraysinstalledPluginsanduninstalledPlugins.- {plugins.map((plugin) => ( + {installedPlugins.map((plugin) => ( <PluginCard key={plugin.id} plugin={plugin} isActive /> ))} - {plugins.map((plugin) => ( + {uninstalledPlugins.map((plugin) => ( <PluginCard key={plugin.id} plugin={plugin} isActive={false} /> ))}Also applies to: 85-87
🧹 Nitpick comments (50)
packages/project-builder-web/src/pages/data/models/_hooks/useEditedModelConfig.tsx (1)
69-82: Consider adding JSDoc documentation for better maintainability.The hook implementation is solid, but could benefit from JSDoc comments explaining:
- The purpose of the hook
- The expected shape of the selector function
- Example usage
Example documentation:
/** * Hook to access and select data from the edited model configuration. * Must be used within an EditedModelContextProvider. * * @param selector - Function to select specific data from the model config * @returns Selected data from the model config * @throws {Error} If used outside of EditedModelContextProvider * * @example * const name = useEditedModelConfig(model => model.name); */ export function useEditedModelConfig<T>( selector: (model: ModelConfig) => T, ): T { // ... existing implementation }packages/project-builder-web/src/pages/plugins/plugin-config.page.tsx (1)
72-86: Consider adding error handling for save failures.While the success path is well-handled with feedback and navigation, there's no explicit handling of save failures. Consider adding an
onErrorcallback to handle failure scenarios gracefully.saveDefinitionWithFeedbackSync( (draft) => { draft.plugins = (draft.plugins ?? []).filter( (p) => p.id !== metadata.id, ); }, { successMessage: `Disabled ${metadata.displayName}!`, onSuccess: () => { navigate('/plugins'); }, + onError: (error) => { + // Handle the error appropriately + console.error('Failed to disable plugin:', error); + }, }, );packages/project-builder-web/src/pages/apps/edit/admin/AdminGeneralForm.tsx (3)
42-48: Consider adding type annotation for better code clarity.The submit handler implementation is clean and functional. Consider adding a type annotation for the draft config parameter to improve code clarity and maintainability.
- saveDefinitionWithFeedback((draftConfig) => { + saveDefinitionWithFeedback((draftConfig: typeof definition) => {
52-55: Consider adding null coalescing for role mapping.To handle cases where
definition.authmight be undefined, consider adding null coalescing to provide a default empty array.- const roleOptions = definition.auth?.roles.map((role) => ({ + const roleOptions = definition.auth?.roles?.map((role) => ({
58-81: Consider extracting string literals to constants.The form UI is well-structured with proper disabled state handling. Consider extracting string literals (labels, etc.) to constants for better maintainability and potential internationalization.
+ const FORM_LABELS = { + NAME: 'Name', + PACKAGE_LOCATION: 'Package Location (optional) e.g. packages/web', + ALLOWED_ROLES: 'Allowed Roles?', + SAVE: 'Save', + } as const; return ( <div className={clsx('', className)}> <form onSubmit={onSubmit} className="space-y-4"> <TextInput.LabelledController - label="Name" + label={FORM_LABELS.NAME} control={control} name="name" /> <TextInput.LabelledController - label="Package Location (optional) e.g. packages/web" + label={FORM_LABELS.PACKAGE_LOCATION} control={control} name="packageLocation" /> {roleOptions && ( <CheckedArrayInput.LabelledController - label="Allowed Roles?" + label={FORM_LABELS.ALLOWED_ROLES} control={control} options={roleOptions} name="allowedRoles" /> )} <Button type="submit" disabled={isSavingDefinition}> - Save + {FORM_LABELS.SAVE} </Button>packages/project-builder-web/src/pages/settings/hierarchy.tsx (2)
43-48: Consider combining reference check with the save operation.There's a potential race condition between the reference check and the actual deletion. If the project definition changes between these operations, the reference check results might be outdated.
Consider moving the reference check into the save operation:
- // try deleting the feature - const result = definitionContainer.fixRefDeletions(deleteFeature); - if (result.type === 'failure') { - showRefIssues({ issues: result.issues }); - return; - } + const checkAndDelete = (draftConfig: ProjectDefinition): void => { + const result = definitionContainer.checkRefDeletions(draftConfig, deleteFeature); + if (result.type === 'failure') { + throw new Error('Reference check failed'); + } + deleteFeature(draftConfig); + }; requestConfirm({ title: 'Delete Feature', content: `Are you sure you want to delete ${feature.name}?`, onConfirm: () => { - saveDefinitionWithFeedbackSync(deleteFeature, { + saveDefinitionWithFeedbackSync(checkAndDelete, { successMessage: `Feature ${feature.name} removed`, + onError: (error) => { + if (error.message === 'Reference check failed') { + showRefIssues({ issues: result.issues }); + } + } }); }, });
126-132: Enhance accessibility with ARIA labels and keyboard support.The button lacks proper accessibility attributes for screen readers and keyboard navigation.
Apply these improvements:
<button className="mr-1 flex w-full flex-row items-center space-x-3" + aria-label={`Edit feature ${featureDisplayName}`} + role="button" + tabIndex={0} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + setFeatureToEdit(feature); + setShowFeatureForm(true); + } + }} onClick={() => { setFeatureToEdit(feature); setShowFeatureForm(true); }} >packages/project-builder-web/src/pages/apps/edit/admin/crud/AdminCrudSectionForm.tsx (1)
33-36: LGTM! Consider adding type safety.The direct access to
definition.modelssimplifies the code. Consider adding type annotations for better type safety:- const modelOptions = definition.models.map((model) => ({ + const modelOptions = definition.models.map((model): { label: string; value: string } => ({ label: model.name, value: model.id, }));packages/project-builder-web/src/pages/data/models/_hooks/useModelForm.ts (1)
129-164: Consider refactoring service method validation to reduce duplication.The validation logic for create and update methods is nearly identical. Consider extracting this into a reusable function to improve maintainability.
Here's a suggested refactor:
const validateServiceMethod = ( method: { enabled?: boolean; fields?: any[]; transformerNames?: any[] }, methodName: string ): boolean => { if (method?.enabled) { if (!method.fields?.length && !method.transformerNames?.length) { toast.error( `${methodName} method must have at least one field or transformer.` ); return false; } } return true; }; // Usage: if (!validateServiceMethod(service.create, 'Create')) return; if (!validateServiceMethod(service.update, 'Update')) return;packages/project-builder-server/src/api/context.ts (1)
7-24: Consider using a map-like structure for frequent service lookups.This implementation works fine for smaller numbers of services. However, if
apiContext.servicesgrows large or if lookups happen frequently, using a dictionary or mapping fromprojectIdto service might be more performant and explicit.packages/project-builder-server/src/api/projects.ts (1)
8-12: Consider adding JSDoc or comments for ProjectInfo.Documenting the purpose of each field (e.g.,
id,name,directory) will improve clarity and maintainability.packages/project-builder-lib/src/migrations/index.ts (1)
18-26: Validate schemaVersion type for extra safety.The
isMigrateableProjectDefinitionguard is a strong check. Optionally, consider confirming thatschemaVersionis a number to reduce runtime errors down the line.return ( typeof projectDefinition === 'object' && !!projectDefinition && 'schemaVersion' in projectDefinition + && typeof (projectDefinition as ProjectDefinition).schemaVersion === 'number' );packages/project-builder-web/src/app/ClientVersionProvider/ClientVersionProvider.tsx (2)
26-35: Consider async/await for readability.The
fetchVersionfunction is correct and logically sound. However, using an async/await pattern inside a try/catch can improve readability and more explicitly handle errors:- const fetchVersion = useCallback(() => { - getVersionInfo() - .then((version) => { - setClientVersionInfo(version); - }) - .catch((error_: unknown) => { - logError(error_); - setError(error_ as Error); - }); - }, []); + const fetchVersion = useCallback(async () => { + try { + const version = await getVersionInfo(); + setClientVersionInfo(version); + } catch (error_: unknown) { + logError(error_); + setError(error_ as Error); + } + }, []);
45-45: Consider user experience of full reload.Reloading the entire page when a version changes can disrupt a user's workflow. Depending on release requirements, you might explore in-app notifications or partial refresh to avoid losing user context.
packages/project-builder-lib/src/references/fix-ref-deletions.ts (1)
50-72: CallingparseSchemaWithReferencesinside a loop.
- This pattern is effective when references shift after each deletion fix.
- The return block at lines 60–71 properly splits success vs. aggregated failure results.
- Confirm that a single pass is sufficient for typical references and that repeated parsing doesn’t introduce performance concerns for large objects.
packages/project-builder-web/src/app/ProjectDefinitionProvider/ProjectDefinitionProvider.tsx (1)
150-157: Dependency array for useMemo
Ensure that all relevant references are included here. Currently, standard usage, but watch for subtle data flows that might cause stale closures.packages/project-builder-server/src/service/builder-service.ts (4)
11-16: Consider consolidating imports for readability.
You can combine the imports from@halfdomelabs/utilsand@halfdomelabs/utils/nodeto simplify the import statements.
28-40: Good introduction of a file payload interface.
Storingcontentsand itshashis an effective way to maintain integrity checks. Consider whether you also need metadata such as file size or timestamps for auditing purposes.
138-146: Initial definition fallback is helpful.
Providing a default JSON structure when the file is missing prevents errors in the rest of the flow. One tiny improvement might be to log when a new definition is created from scratch.
187-217: Write method with hash comparison mitigates overwriting conflicts.
Returning'original-contents-mismatch'is a clear way to signal that the file was updated externally. Consider whether versioning or more detailed conflict resolution is needed if merges must be handled.packages/project-builder-web/src/pages/features/auth/index.tsx (2)
46-61: Memoized model change detection.
UsingdiffModelinsideuseMemois efficient. If the user or user role references do not exist, returning early prevents TypeError. Consider logging or showing a warning when references are undefined.
115-115: Computed model options.
Mappingdefinition.modelsfor user selection is straightforward. If model definitions become large, consider memoizing for performance.packages/project-builder-lib/src/tools/model-merger/model-merger.ts (1)
174-231: diffModel function is cohesive.
Nicely resolves references for relations, confirms changes in primary key, and returns a structured diff. Consider logging or providing more context if some references are not found indefinitionContainer.packages/project-builder-lib/src/references/ref-builder.ts (2)
28-32: Cross-realm Promise check consideration.
object instanceof Promisemay fail if a promise originates from another realm (e.g., an iframe). As a minor enhancement, you might consider checkingtypeof object?.then === 'function'.
50-67: Interface design is clear.
DefinitionEntityNameResolveris well-structured. If asynchronous name resolution is ever required, consider allowing an async return type.packages/project-builder-web/src/services/api/sync.ts (1)
5-18: Remove unused payload parameter.The function documentation mentions a
payloadparameter, but it's not used in the function signature or implementation.Apply this diff to remove the unused parameter from the documentation:
/** * Starts a sync operation for a project. * * @param id - The ID of the project to sync. - * @param payload - The payload to sync. */packages/project-builder-lib/src/references/parse-schema-with-references.ts (1)
11-27: Consider enhancing error handling.The function is well-documented and implements a clean parsing flow. However, consider wrapping the parsing operation in a try-catch block to provide more context-specific error messages.
export function parseSchemaWithReferences<T extends z.ZodTypeAny>( schema: T, input: unknown, options?: ResolveZodRefPayloadNamesOptions, ): ResolvedZodRefPayload<z.output<T>> { + try { const wrapper = ZodRefWrapper.create(schema); return resolveZodRefPayloadNames(wrapper.parse(input), options); + } catch (error) { + throw new Error(`Failed to parse schema with references: ${error.message}`, { cause: error }); + } }packages/project-builder-web/src/services/api/errors.ts (1)
18-27: Add explicit return type annotation.The function's return type should be explicitly annotated for better code documentation and type safety.
export function createProjectNotFoundHandler( projectId: string, -): (err: unknown) => never { +): (err: unknown) => never | ProjectNotFoundError { return (err: unknown) => { if (isTRPCClientError(err) && err.data?.code === 'NOT_FOUND') { throw new ProjectNotFoundError(projectId); } throw err; }; }packages/project-builder-web/src/pages/features/auth/auth-models.ts (1)
34-63: Consider adding input validation for userModelRef.The
createAuth0Modelsfunction correctly defines the models and their relationships. However, consider adding validation for theuserModelRefparameter to ensure it's not empty or invalid.export function createAuth0Models(userModelRef: string): { user: ModelDefinitionInput; userRole: ModelDefinitionInput; } { + if (!userModelRef?.trim()) { + throw new Error('userModelRef must be a non-empty string'); + } return { user: { fields: AUTH0_MODEL_FIELDS,packages/project-builder-web/src/app/ProjectDefinitionProvider/services/parse-project-definition-contents.ts (1)
21-25: Consider improving error message specificity.The error message for invalid project definition could be more specific about what makes it invalid.
- throw new UserVisibleError( - 'The project configuration is not valid JSON object. Please check the file and try again.', - ); + throw new UserVisibleError( + 'The project configuration is not a valid project definition. Expected a JSON object with required project configuration fields. Please check the file and try again.', + );packages/project-builder-web/src/app/ProjectDefinitionProvider/services/schema-parser-context-manager.ts (1)
33-48: Consider debouncing plugin asset change handler.The hot reload event handler might trigger frequently during development. Consider debouncing the handler to prevent unnecessary context reloads.
+import debounce from 'lodash/debounce'; + listenForPluginAssetsChanges( onSchemaParserContextUpdated: ( schemaParserContext: SchemaParserContext, ) => void, onError: (error: unknown) => void, ): () => void { - return subscribeToViteHotReloadEvent('plugin-assets-changed', () => { + const debouncedHandler = debounce(() => { this.loadSchemaParserContext() .then((schemaParserContext) => { onSchemaParserContextUpdated(schemaParserContext); }) .catch((error: unknown) => { onError(error); }); - }); + }, 300); + return subscribeToViteHotReloadEvent('plugin-assets-changed', debouncedHandler); }packages/project-builder-web/src/app/ProjectDefinitionProvider/hooks/use-project-definition-container.ts (2)
21-52: Consider memoizing the error handler callback.The implementation is solid, but the error handler could be memoized to prevent unnecessary re-renders.
- const { projectDefinitionContainer, error } = useMemo(() => { + const memoizedErrorHandler = useCallback((err: unknown) => { + return { + projectDefinitionContainer: undefined, + error: err, + }; + }, []); + + const { projectDefinitionContainer, error } = useMemo(() => { try { if (!schemaParserContext || !projectDefinitionFilePayload) { return { projectDefinitionContainer: undefined, error: undefined, }; } const definitionContainer = parseProjectDefinitionContents( projectDefinitionFilePayload.contents, schemaParserContext, ); return { projectDefinitionContainer: { container: definitionContainer, hash: projectDefinitionFilePayload.hash, }, error: undefined, }; } catch (err) { - return { - projectDefinitionContainer: undefined, - error: err, - }; + return memoizedErrorHandler(err); } }, [schemaParserContext, projectDefinitionFilePayload]);
54-58: Consider using a logging service instead of console.error.Direct console logging in production code should be replaced with a proper logging service for better error tracking and management.
useEffect(() => { if (error) { - console.error(error); + logService.error('Project definition parsing error:', error); } }, [error]);packages/project-builder-web/src/pages/apps/new.page.tsx (2)
41-54: Fix typo in success message.There's a typo in the success toast message.
- toast.success(`Sucessfully created ${data.name}!`); + toast.success(`Successfully created ${data.name}!`);
74-76: Consider adding loading state indicator.The button is correctly disabled during saving, but users might benefit from visual feedback.
- <Button type="submit" disabled={isSavingDefinition}> - Create + <Button type="submit" disabled={isSavingDefinition}> + {isSavingDefinition ? 'Creating...' : 'Create'} </Button>packages/project-builder-web/src/pages/data/models/edit/[id]/index.page.tsx (2)
40-42: Address TODO comments regarding validation.The TODO comments indicate missing validation for field types and SET NULL constraints.
Would you like me to help implement the validation logic for:
- Field type and referenced type matching
- SET NULL validation for non-optional fields?
51-64: Consider adding form error boundary.The form is wrapped in an ErrorBoundary but might benefit from a more specific form error handling component.
<form onSubmit={onSubmit} className="min-w-[700px] max-w-7xl flex-1 space-y-4 px-4 pb-4" > + <FormErrorBoundary> <ModelFieldsForm control={control} setValue={setValue} /> <SectionList> <ModelRelationsSection control={control} setValue={setValue} /> <ModelUniqueConstraintsSection control={control} setValue={setValue} /> </SectionList> <FormActionBar form={form} /> + </FormErrorBoundary> </form>packages/project-builder-web/src/pages/data/models/edit/[id]/_components/fields/ModelFieldTypeInput.tsx (1)
26-34: LGTM! Clean refactor of enum options retrieval.The change from
parsedProject.getEnums()todefinition.enumsaligns with the PR's objective of improving the project definition flow.Consider adding type assertion or runtime validation for
definition.enumsto ensure type safety:- const enumOptions: ScalarFieldTypeOption[] = (definition.enums ?? []).map( + const enumOptions: ScalarFieldTypeOption[] = ((definition.enums ?? []) as Array<{ name: string; id: string }>).map(packages/project-builder-web/src/services/api/project-definition.ts (1)
69-80: Consider adding error handling for subscription.The subscription setup looks good, but consider handling potential subscription errors.
export function listenForProjectDefinitionChanges( id: string, onData: (value: ProjectDefinitionFilePayload) => void, ): () => void { const result = trpc.projects.onProjectJsonChanged.subscribe( { id }, - { onData }, + { + onData, + onError: (error) => { + console.error('Project definition subscription error:', error); + }, + }, ); return () => { result.unsubscribe(); }; }packages/project-builder-lib/src/schema/models/transformers/built-in-transformers.ts (1)
66-71: LGTM! Enhanced name resolution for embedded relations.The use of createDefinitionEntityNameResolver provides a more flexible and maintainable approach to name resolution.
Consider adding JSDoc comments to explain the name resolution strategy:
+/** + * Resolves the transformer name based on the foreign relation reference. + * This ensures that the transformer name stays in sync with its related entity. + */ getNameResolver: (entity) => createDefinitionEntityNameResolver({ idsToResolve: { foreignRelation: entity.foreignRelationRef }, resolveName: (entityNames) => entityNames.foreignRelation, }),packages/project-builder-lib/src/web/hooks/useProjectDefinition.ts (1)
16-20: Consider adding JSDoc for SaveDefinitionWithFeedbackOptions.The interface would benefit from JSDoc documentation explaining each option's purpose and behavior.
Add documentation like this:
+/** + * Options for saving project definition with feedback. + * + * @property disableDeleteRefDialog - Whether to disable the delete reference confirmation dialog + * @property successMessage - Custom success message to show in the toast + * @property onSuccess - Callback function to execute after successful save + */ export interface SaveDefinitionWithFeedbackOptions { disableDeleteRefDialog?: boolean; successMessage?: string; onSuccess?: () => void; }packages/project-builder-lib/src/references/resolve-zod-ref-payload-names.ts (1)
52-64: Optimize name resolution with memoization.The
resolveIdToNamefunction could benefit from memoization to avoid redundant resolutions of the same IDs.+ const resolvedNamesCache = new Map<string, string>(); function resolveIdToName(id: string): string { if (skipReferenceNameResolution) { return id; } + const cached = resolvedNamesCache.get(id); + if (cached) { + return cached; + } const entity = resolvedEntitiesById.get(id); if (!entity) { if (allowInvalidReferences) { return id; } throw new Error(`Could not resolve entity name for id: ${id}`); } - return entity.name; + resolvedNamesCache.set(id, entity.name); + return entity.name; }packages/project-builder-web/src/app/ProjectDefinitionProvider/hooks/use-remote-project-definition-contents.ts (2)
26-58: Consider adding error retry logic for network failures.While the error handling is good, network failures during downloadDefinitionContents could benefit from retry logic, especially in unstable network conditions.
projectDefinitionFileManager .downloadDefinitionContents() + .retry({ + retries: 3, + minTimeout: 1000, + factor: 2, + }) .then((payload) => { setProjectDefinitionFilePayload(payload); })
60-79: Consider adding loading state for upload operation.The upload operation could benefit from a loading state to prevent multiple simultaneous uploads and provide better UX feedback.
const uploadProjectDefinitionContents = useCallback( async (newContents: string) => { + const [isUploading, setIsUploading] = useState(false); if (!projectDefinitionFileManager) { throw new Error('Project definition file manager not found'); } + if (isUploading) { + return; + } + setIsUploading(true); try { await projectDefinitionFileManager .uploadDefinitionContents(newContents) .then((newPayload) => { setProjectDefinitionFilePayload(newPayload); }) .catch((err: unknown) => { if (err instanceof ProjectNotFoundError) { resetCurrentProjectId(); } throw err; }); + } finally { + setIsUploading(false); + } }, [projectDefinitionFileManager, resetCurrentProjectId], );packages/project-builder-web/src/pages/apps/edit/WebAppForm.tsx (2)
31-39: Consider adding optimistic updates.The form submission could benefit from optimistic updates to improve perceived performance.
const onSubmit = handleSubmit((data) => + // Optimistically update the UI + setProjectDefinitionFilePayload((prev) => ({ + ...prev, + apps: prev.apps.map((app) => + app.id === appConfig.id ? data : app, + ), + })), saveDefinitionWithFeedback((draftConfig) => { draftConfig.apps = draftConfig.apps.map((app) => app.id === appConfig.id ? data : app, ); }), );
99-101: Add loading indicator to the Save button.The button's disabled state is good, but a loading indicator would provide better UX feedback.
-<Button type="submit" disabled={isSavingDefinition}> +<Button + type="submit" + disabled={isSavingDefinition} + icon={isSavingDefinition ? <Spinner size="sm" /> : undefined} +> Save </Button>packages/project-builder-lib/src/references/deserialize-schema.ts (2)
18-21: Consider adding validation for skipReferenceNameResolution.The skipReferenceNameResolution option is crucial for correct deserialization. Consider adding validation to ensure it's always true in this context.
const payload = parseSchemaWithReferences(schema, input, { skipReferenceNameResolution: true, }); + +// Ensure name resolution was skipped +if (!payload.skipReferenceNameResolution) { + throw new Error('Reference name resolution must be skipped for deserialization'); +}
27-44: Consider extracting duplicate ID validation to a separate function.The duplicate ID validation logic is complex and could benefit from being extracted into a separate function for better maintainability.
+function validateUniqueEntityIds(entities: DefinitionEntity[]): void { + const entitiesById = groupBy(entities, (entity) => entity.id); + const duplicateEntityIds = Object.values(entitiesById).filter( + (e) => e.length > 1, + ); + if (duplicateEntityIds.length > 0) { + throw new Error( + `Found multiple duplicate entity IDs: ${duplicateEntityIds + .map( + (ents) => + `${ents[0].id} (${ents.map((e) => e.path.join('.')).join(',')})`, + ) + .join(', ')}`, + ); + } + const uniqueEntityIds = uniq(entities.map((e) => e.id)); + if (uniqueEntityIds.length !== entities.length) { + throw new Error(`Found duplicate entity IDs`); + } +} + +// Use the extracted function +validateUniqueEntityIds(entities);packages/project-builder-web/src/pages/apps/edit/admin/AdminEditSectionForm.tsx (1)
74-98: Consider adding loading feedback.While the save operation is in progress, consider showing a loading indicator or progress message to improve user experience during potentially long operations.
return saveDefinitionWithFeedback( (config) => { const adminApp = config.apps.find((app) => app.id === appConfig.id); if (adminApp?.type !== 'admin') { throw new Error('Cannot add a section to a non-admin app'); } adminApp.sections = sortBy( [ ...(adminApp.sections ?? []).filter( (section) => !sectionId || section.id !== sectionId, ), { ...data, id }, ], [(section) => section.name], ); }, { + loadingMessage: 'Saving section...', onSuccess: () => { if (!sectionId) { navigate(`edit/${adminSectionEntityType.toUid(id)}`); } }, }, );packages/project-builder-web/src/pages/settings/theme-builder.tsx (1)
106-114: Clarify or remove the TODO comment.The TODO comment suggests that this alert might need to be removed, but it's unclear why. Please clarify the conditions under which this alert should be removed or remove it if it's no longer needed.
Would you like me to help track this TODO by creating a new issue?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (107)
.changeset/afraid-foxes-build.md(1 hunks)packages/project-builder-lib/src/compiler/app-compiler-spec.ts(1 hunks)packages/project-builder-lib/src/definition/project-definition-container.ts(5 hunks)packages/project-builder-lib/src/index.ts(0 hunks)packages/project-builder-lib/src/migrations/index.ts(2 hunks)packages/project-builder-lib/src/parser/index.ts(1 hunks)packages/project-builder-lib/src/parser/parser.ts(2 hunks)packages/project-builder-lib/src/parser/plugins/auth0.ts(0 hunks)packages/project-builder-lib/src/parser/types.ts(0 hunks)packages/project-builder-lib/src/plugins/schema/parser.ts(1 hunks)packages/project-builder-lib/src/references/deserialize-schema.ts(2 hunks)packages/project-builder-lib/src/references/fix-ref-deletions.ts(2 hunks)packages/project-builder-lib/src/references/parse-schema-with-references.ts(1 hunks)packages/project-builder-lib/src/references/ref-builder.ts(20 hunks)packages/project-builder-lib/src/references/ref-builder.unit.test.ts(1 hunks)packages/project-builder-lib/src/references/resolve-zod-ref-payload-names.ts(1 hunks)packages/project-builder-lib/src/references/resolve-zod-ref-payload-names.unit.test.ts(1 hunks)packages/project-builder-lib/src/references/serialize-schema.ts(2 hunks)packages/project-builder-lib/src/references/types.ts(4 hunks)packages/project-builder-lib/src/schema/models/index.ts(1 hunks)packages/project-builder-lib/src/schema/models/transformers/built-in-transformers.ts(3 hunks)packages/project-builder-lib/src/tools/model-merger/index.ts(1 hunks)packages/project-builder-lib/src/tools/model-merger/model-merger.ts(1 hunks)packages/project-builder-lib/src/web/hooks/useBlockUnsavedChangesNavigate.ts(2 hunks)packages/project-builder-lib/src/web/hooks/useProjectDefinition.ts(1 hunks)packages/project-builder-lib/src/web/hooks/useResettableForm.ts(3 hunks)packages/project-builder-server/src/api/context.ts(1 hunks)packages/project-builder-server/src/api/index.ts(1 hunks)packages/project-builder-server/src/api/plugins.ts(1 hunks)packages/project-builder-server/src/api/projects.ts(1 hunks)packages/project-builder-server/src/api/sync.ts(1 hunks)packages/project-builder-server/src/api/version.ts(1 hunks)packages/project-builder-server/src/compiler/admin/crud/displays.ts(3 hunks)packages/project-builder-server/src/compiler/app-entry-builder.ts(0 hunks)packages/project-builder-server/src/compiler/backend/enums.ts(2 hunks)packages/project-builder-server/src/compiler/backend/fastify.ts(1 hunks)packages/project-builder-server/src/compiler/backend/feature.ts(2 hunks)packages/project-builder-server/src/compiler/backend/services.ts(3 hunks)packages/project-builder-server/src/runner/index.ts(1 hunks)packages/project-builder-server/src/server/plugin.ts(4 hunks)packages/project-builder-server/src/service/builder-service.ts(4 hunks)packages/project-builder-server/src/utils/typed-event-emitter.ts(0 hunks)packages/project-builder-web/package.json(2 hunks)packages/project-builder-web/public/preview-app.json(15 hunks)packages/project-builder-web/src/app/App.tsx(2 hunks)packages/project-builder-web/src/app/AppLayout/AppLayout.tsx(1 hunks)packages/project-builder-web/src/app/AppLayout/ProjectSyncModal.tsx(3 hunks)packages/project-builder-web/src/app/ClientVersionProvider/ClientVersionProvider.test.tsx(1 hunks)packages/project-builder-web/src/app/ClientVersionProvider/ClientVersionProvider.tsx(2 hunks)packages/project-builder-web/src/app/ProjectDefinitionProvider/NewProjectCard.tsx(4 hunks)packages/project-builder-web/src/app/ProjectDefinitionProvider/ProjectDefinitionProvider.tsx(1 hunks)packages/project-builder-web/src/app/ProjectDefinitionProvider/hooks/use-project-definition-container.ts(1 hunks)packages/project-builder-web/src/app/ProjectDefinitionProvider/hooks/use-remote-project-definition-contents.ts(1 hunks)packages/project-builder-web/src/app/ProjectDefinitionProvider/hooks/use-schema-parser-context.ts(1 hunks)packages/project-builder-web/src/app/ProjectDefinitionProvider/services/parse-project-definition-contents.ts(1 hunks)packages/project-builder-web/src/app/ProjectDefinitionProvider/services/project-definition-file-manager.ts(1 hunks)packages/project-builder-web/src/app/ProjectDefinitionProvider/services/schema-parser-context-manager.ts(1 hunks)packages/project-builder-web/src/app/ProjectSelectorGate/ProjectSelectorDialog.tsx(2 hunks)packages/project-builder-web/src/app/ProjectSelectorGate/ProjectSelectorGate.tsx(2 hunks)packages/project-builder-web/src/app/components/ProjectDefinitionGate.tsx(0 hunks)packages/project-builder-web/src/components/Console/index.tsx(2 hunks)packages/project-builder-web/src/components/FormActionBar/index.tsx(3 hunks)packages/project-builder-web/src/hooks/useClientVersion.ts(0 hunks)packages/project-builder-web/src/hooks/useProjects.ts(1 hunks)packages/project-builder-web/src/hooks/useRemoteProjectDefinition.ts(0 hunks)packages/project-builder-web/src/pages/apps/AppsLayout.page.tsx(1 hunks)packages/project-builder-web/src/pages/apps/edit.page.tsx(3 hunks)packages/project-builder-web/src/pages/apps/edit/BackendAppForm.tsx(2 hunks)packages/project-builder-web/src/pages/apps/edit/WebAppForm.tsx(2 hunks)packages/project-builder-web/src/pages/apps/edit/admin/AdminEditSectionForm.tsx(7 hunks)packages/project-builder-web/src/pages/apps/edit/admin/AdminGeneralForm.tsx(2 hunks)packages/project-builder-web/src/pages/apps/edit/admin/crud/AdminCrudEmbeddedForm.tsx(1 hunks)packages/project-builder-web/src/pages/apps/edit/admin/crud/AdminCrudSectionForm.tsx(1 hunks)packages/project-builder-web/src/pages/apps/edit/admin/crud/CrudTableColumnsForm.tsx(2 hunks)packages/project-builder-web/src/pages/apps/list.page.tsx(1 hunks)packages/project-builder-web/src/pages/apps/new.page.tsx(3 hunks)packages/project-builder-web/src/pages/data/enums/edit/components/EnumHeaderBar.tsx(3 hunks)packages/project-builder-web/src/pages/data/enums/edit/components/EnumInfoEditDialog.tsx(2 hunks)packages/project-builder-web/src/pages/data/enums/edit/edit.page.tsx(2 hunks)packages/project-builder-web/src/pages/data/enums/hooks/useEnumForm.tsx(5 hunks)packages/project-builder-web/src/pages/data/enums/new/NewEnumDialog.tsx(2 hunks)packages/project-builder-web/src/pages/data/models/_hooks/useEditedModelConfig.tsx(1 hunks)packages/project-builder-web/src/pages/data/models/_hooks/useModelForm.ts(4 hunks)packages/project-builder-web/src/pages/data/models/edit/[id]/_components/ModelHeaderBar.tsx(3 hunks)packages/project-builder-web/src/pages/data/models/edit/[id]/_components/fields/ModelFieldTypeInput.tsx(1 hunks)packages/project-builder-web/src/pages/data/models/edit/[id]/graphql.page.tsx(1 hunks)packages/project-builder-web/src/pages/data/models/edit/[id]/index.page.tsx(1 hunks)packages/project-builder-web/src/pages/data/models/edit/[id]/service.page.tsx(1 hunks)packages/project-builder-web/src/pages/features/auth/auth-models.ts(1 hunks)packages/project-builder-web/src/pages/features/auth/index.tsx(4 hunks)packages/project-builder-web/src/pages/plugins/PluginCard.tsx(5 hunks)packages/project-builder-web/src/pages/plugins/home.page.tsx(3 hunks)packages/project-builder-web/src/pages/plugins/plugin-config.page.tsx(3 hunks)packages/project-builder-web/src/pages/settings/components/FeatureForm.tsx(6 hunks)packages/project-builder-web/src/pages/settings/hierarchy.tsx(2 hunks)packages/project-builder-web/src/pages/settings/project-settings.tsx(2 hunks)packages/project-builder-web/src/pages/settings/theme-builder.tsx(4 hunks)packages/project-builder-web/src/services/api/errors.ts(1 hunks)packages/project-builder-web/src/services/api/index.ts(1 hunks)packages/project-builder-web/src/services/api/plugins.ts(1 hunks)packages/project-builder-web/src/services/api/project-definition.ts(1 hunks)packages/project-builder-web/src/services/api/projects.ts(1 hunks)packages/project-builder-web/src/services/api/sync.ts(1 hunks)packages/project-builder-web/src/services/api/version.ts(1 hunks)packages/project-builder-web/src/services/config.ts(1 hunks)packages/project-builder-web/src/services/error-formatter.ts(1 hunks)packages/project-builder-web/src/services/hot-loader.ts(1 hunks)
⛔ Files not processed due to max files limit (23)
- packages/project-builder-web/src/services/remote.ts
- packages/project-builder-web/src/services/trpc.ts
- packages/project-builder-web/src/tests/setup.ts
- packages/project-builder-web/src/utils/typed-event-emitter.ts
- packages/project-builder-web/vitest.config.ts
- packages/sync/src/utils/evented-logger.ts
- packages/sync/src/utils/index.ts
- packages/sync/src/utils/typed-event-emitter.ts
- packages/tools/eslint-configs/react.js
- packages/tools/eslint-configs/tailwind.js
- packages/ui-components/package.json
- packages/ui-components/src/components/ErrorDisplay/ErrorDisplay.tsx
- packages/ui-components/src/components/Loader/Loader.tsx
- packages/ui-components/src/components/Toaster/Toaster.tsx
- packages/utils/src/crypto/hash.ts
- packages/utils/src/crypto/index.ts
- packages/utils/src/events/index.ts
- packages/utils/src/events/typed-event-emitter.ts
- packages/utils/src/events/typed-event-emitter.unit.test.ts
- packages/utils/src/index.ts
- plugins/baseplate-plugin-storage/src/storage/core/components/CategoryEditorForm.tsx
- plugins/baseplate-plugin-storage/src/storage/core/components/StorageConfig.tsx
- plugins/baseplate-plugin-storage/src/storage/transformers/types.ts
💤 Files with no reviewable changes (8)
- packages/project-builder-lib/src/index.ts
- packages/project-builder-server/src/compiler/app-entry-builder.ts
- packages/project-builder-web/src/app/components/ProjectDefinitionGate.tsx
- packages/project-builder-lib/src/parser/plugins/auth0.ts
- packages/project-builder-server/src/utils/typed-event-emitter.ts
- packages/project-builder-lib/src/parser/types.ts
- packages/project-builder-web/src/hooks/useClientVersion.ts
- packages/project-builder-web/src/hooks/useRemoteProjectDefinition.ts
✅ Files skipped from review due to trivial changes (5)
- .changeset/afraid-foxes-build.md
- packages/project-builder-lib/src/parser/index.ts
- packages/project-builder-web/src/app/AppLayout/AppLayout.tsx
- packages/project-builder-web/src/services/api/index.ts
- packages/project-builder-lib/src/tools/model-merger/index.ts
🔇 Additional comments (209)
packages/project-builder-web/src/pages/settings/project-settings.tsx (2)
27-35: Great optimization using useMemo for form values!The memoization of
defaultValuesprevents unnecessary recalculations when the component re-renders, improving performance. The values are properly derived from the definition using thepickutility.
39-43: Clean implementation of the new save flow!The new submission logic using
saveDefinitionWithFeedbackaligns perfectly with the PR's objective to implement asynchronous saving capabilities. The use ofObject.assignfor updating the draft config is clean and efficient.packages/project-builder-web/src/pages/data/models/_hooks/useEditedModelConfig.tsx (1)
74-76: LGTM! Error message now accurately reflects the required context.The updated error message provides clearer guidance by correctly referencing the
EditedModelContextProvider, improving the developer experience.packages/project-builder-web/src/pages/plugins/plugin-config.page.tsx (3)
18-24: LGTM! Good improvements to state management.The addition of
saveDefinitionWithFeedbackSyncandisSavingDefinitionenhances the user experience by providing better feedback during save operations.
107-109: LGTM! Good UX improvement.The use of
isSavingDefinitionto disable the button during save operations prevents potential race conditions from multiple submissions.
88-92: Verify the implementation ofonSave.The
onSavefunction only navigates away if there's no plugin definition but doesn't perform any actual save operation. This seems incomplete or incorrect given the function name and its usage in theContainercomponent.Please verify:
- Should this function actually save something?
- Is this a leftover from previous code?
- Is the save functionality handled within the
Containercomponent instead?packages/project-builder-lib/src/web/hooks/useBlockUnsavedChangesNavigate.ts (3)
2-2: LGTM! Import changes align with the new form state management approach.The addition of
Controltype anduseFormStatehook from react-hook-form supports the refactored form state management.
7-17: LGTM! Function signature changes follow React Hook Form best practices.The refactor to use
controlinstead offormStateis a good improvement as it:
- Follows React Hook Form's recommended patterns
- Enables internal form state management
- Maintains type safety through generics
18-20: LGTM! Form state management is now better encapsulated.The internal derivation of form state using
useFormStatehook improves the component's encapsulation while maintaining the same functionality.packages/project-builder-web/src/pages/apps/edit/admin/AdminGeneralForm.tsx (2)
25-30: LGTM! Improved state management with better feedback mechanisms.The updated
useProjectDefinitionhook destructuring provides clearer access to the project definition state and saving functionality, aligning well with the PR's objectives of improving the sync/save flow.
36-40: LGTM! More controlled form initialization.The switch from
defaultValuestovaluesinuseResettableFormprovides better control over form initialization, ensuring the form always reflects the currentappConfig.packages/project-builder-web/src/pages/settings/hierarchy.tsx (1)
24-25: LGTM! Good use of the new save flow.The change to use
saveDefinitionWithFeedbackSyncaligns well with the PR's objective of improving the synchronization and saving process.packages/project-builder-web/src/pages/apps/edit/admin/crud/AdminCrudSectionForm.tsx (1)
31-31:Details
✅ Verification successful
LGTM! Verify consistent usage across components.
The change to use
definitiondirectly fromuseProjectDefinitionaligns with the broader refactor of project definition handling.Let's verify that this pattern is consistently applied across other components:
🏁 Script executed:
#!/bin/bash # Description: Check for consistent usage of `definition` across components. # Test: Search for any remaining instances of `parsedProject`. Expect: No occurrences. rg -A 5 $'parsedProject.*=.*useProjectDefinition' # Test: Search for the new pattern. Expect: Multiple occurrences showing consistent usage. rg -A 5 $'definition.*=.*useProjectDefinition'Length of output: 26265
LGTM – Consistent usage confirmed.
The change in
AdminCrudSectionForm.tsxto extractdefinitiondirectly fromuseProjectDefinition()is consistent with how other components now access project definitions. The executed script confirms that outdatedparsedProjectreferences have been fully removed and that the new pattern (definitionordefinitionContainer, as contextually required) is uniformly applied across the codebase.packages/project-builder-web/src/pages/data/models/_hooks/useModelForm.ts (2)
23-23: LGTM! Improved error handling with dedicated error types.The changes enhance error handling by using a dedicated NotFoundError type and a unified saveDefinitionWithFeedback mechanism.
Also applies to: 64-64
166-196: LGTM! Well-structured definition saving with proper feedback.The implementation provides a clear and consistent user experience with:
- Operation-specific success messages
- Proper form state management
- Appropriate navigation after success
packages/project-builder-web/src/components/FormActionBar/index.tsx (3)
7-12: LGTM! Well-structured interface extension.The addition of the optional
disabledprop enhances the component's flexibility for handling asynchronous operations.
14-17: LGTM! Clean component signature update.The destructuring pattern is well-implemented and maintains type safety.
30-30:Details
✅ Verification successful
Consider extracting the disabled logic to reduce duplication.
The disabled state logic is duplicated between both buttons. Consider extracting it to a constant or memoized value.
const FormActionBar = ({ form, disabled, }: FormActionBarProps): React.JSX.Element => { const { formState } = form; const isDirty = Object.keys(formState.dirtyFields).length > 0; + const isDisabled = formState.isSubmitting || !isDirty || disabled; return ( <div className="absolute inset-x-0 bottom-0 z-50 flex min-h-[--action-bar-height] items-center space-x-4 border-t border-border bg-white pl-4"> <Button variant="outline" size="sm" type="button" onClick={() => { form.reset(); }} - disabled={formState.isSubmitting || !isDirty || disabled} + disabled={isDisabled} > Reset </Button> <Button.WithIcon icon={MdOutlineSave} variant="default" size="sm" type="submit" - disabled={formState.isSubmitting || !isDirty || disabled} + disabled={isDisabled} > Save </Button.WithIcon> </div> ); };Let's verify the disabled state handling is consistent with other form components:
Also applies to: 39-39
🏁 Script executed:
#!/bin/bash # Search for other form components using FormActionBar to verify disabled prop usage rg -A 5 "FormActionBar" packages/project-builder-web/src/Length of output: 9901
Refactor: Extract duplicated disabled logic in FormActionBar
The current implementation repeats the same disabled state computation for both the Reset and Save buttons. To improve clarity and reduce duplication, consider extracting this logic into a dedicated constant. This approach has been verified across the codebase and is consistent with how FormActionBar is used in other components.
- Location:
packages/project-builder-web/src/components/FormActionBar/index.tsx(around lines 30).- Action: Define a constant (e.g.,
isDisabled) with the logicformState.isSubmitting || !isDirty || disabledand use it for both buttons.const FormActionBar = ({ form, disabled, }: FormActionBarProps): React.JSX.Element => { const { formState } = form; const isDirty = Object.keys(formState.dirtyFields).length > 0; + const isDisabled = formState.isSubmitting || !isDirty || disabled; return ( <div className="absolute inset-x-0 bottom-0 z-50 flex min-h-[--action-bar-height] items-center space-x-4 border-t border-border bg-white pl-4"> <Button variant="outline" size="sm" type="button" onClick={() => { form.reset(); }} - disabled={formState.isSubmitting || !isDirty || disabled} + disabled={isDisabled} > Reset </Button> <Button.WithIcon icon={MdOutlineSave} variant="default" size="sm" type="submit" - disabled={formState.isSubmitting || !isDirty || disabled} + disabled={isDisabled} > Save </Button.WithIcon> </div> ); };packages/project-builder-web/src/pages/apps/AppsLayout.page.tsx (2)
16-16: LGTM! Simplified data access pattern.The change to access
appsdirectly fromdefinitionimproves code readability while maintaining type safety.
14-14:Details
✅ Verification successful
Verify hook usage across the codebase.
The change from
parsedProjecttodefinitionis part of the broader refactoring effort. Let's verify that all components using this hook have been updated consistently.
🏁 Script executed:
#!/bin/bash # Search for any remaining usage of parsedProject from useProjectDefinition ast-grep --pattern 'const { parsedProject } = useProjectDefinition()'Length of output: 71
Below is a new shell script to further verify that no lingering references to the old property
parsedProjectremain in the repository. Please run the script and share the output (if any) so we can confirm that the refactor has been applied consistently across the codebase.Once confirmed that no results are returned, we can conclude the hook usage has been updated properly.
🏁 Script executed:
[run_scripts] #!/bin/bash echo "Searching for any lingering references to 'parsedProject' in the repository..." rg "parsedProject" -n .Length of output: 318
Consistent Hook Update – No Latent Issues Found
The change from extracting
parsedProjecttodefinitionfrom theuseProjectDefinitionhook is aligned with the refactoring objectives. Our verification (via the searches for"parsedProject") did not reveal any lingering references across the codebase, suggesting that the update has been applied consistently.
- Confirmed that no files still reference
parsedProject.- The rename supports the intended refactor for the sync/save flow.
packages/project-builder-web/src/pages/data/enums/edit/edit.page.tsx (3)
20-20: LGTM! Clean refactor of hook parameters.The refactored
useBlockUnsavedChangesNavigatecall using object parameters improves code readability and maintainability.
32-32: LGTM! Enhanced UX with disabled state.The addition of the
disabledprop toFormActionBarprevents multiple submissions while saving, improving user experience.
15-15:Details
❓ Verification inconclusive
LGTM! Verify formState usage.
The addition of
isSavingDefinitionstate improves UX by tracking form submission state. The removal offormStatefrom destructuring suggests a change in form state management.Also applies to: 18-18
🏁 Script executed:
#!/bin/bash # Description: Check if formState is used elsewhere in the codebase rg -A 5 'formState' packages/project-builder-web/src/pages/data/enumsLength of output: 69
I'll perform an additional check to see if any file within packages/project-builder-web still references
formState. Please run the following shell script to search the entire project for any occurrences of"formState":Once you verify that no results are returned (or that any identified usages are intentional and do not impact the updated state management), you can conclude that the removal of
formStatefrom the destructuring in favor of the newisSavingDefinitionstate is correct.
LGTM! Verified that the removal of
formStateis consistent with the new usage ofisSavingDefinitionto track form submission state. Please ensure manually that no dependent functionality relies onformStatein the project.
🏁 Script executed:
#!/bin/bash # Checking for any usage of 'formState' in the entire project-builder-web directory rg -n "formState" packages/project-builder-webLength of output: 925
LGTM!
- The change in
packages/project-builder-web/src/pages/data/enums/edit/edit.page.tsxto replaceformStatewith the newisSavingDefinitionstate for tracking form submission is intentional.- Our search confirms that while
formStateis still actively used in other parts of the project (e.g., inNewProjectCard,ServiceTransformerForm,FeatureForm, and withinFormActionBar), its removal in this enum editing page appears isolated.- Please ensure that no dependencies in this context expect
formStateand verify that the updated state management meets the intended UX requirements.packages/project-builder-web/src/pages/data/enums/edit/components/EnumInfoEditDialog.tsx (2)
31-31: LGTM! Added form submission state.The addition of
isSavingDefinitionstate enables better control over form submission.
59-61: LGTM! Enhanced UX with disabled state.The disabled state on the submit button prevents multiple submissions and provides visual feedback during save operation.
packages/project-builder-web/src/pages/data/enums/new/NewEnumDialog.tsx (2)
29-29: LGTM! Added form submission state.The addition of
isSavingDefinitionstate enables better control over form submission.
74-76: LGTM! Enhanced UX with disabled state.The disabled state on the create button prevents multiple submissions and provides visual feedback during save operation.
packages/project-builder-server/src/api/context.ts (2)
3-5: Imports look good.These imports correctly bring in
TRPCErrorfor error handling and theBaseplateApiContexttype without any apparent issues.
27-29: Context type definition appears correct.Defining the
Contexttype as the awaited return of the function ensures proper typing. No further issues found.packages/project-builder-server/src/api/projects.ts (4)
4-4: Import of ProjectDefinitionFilePayload is correct.This import aligns with your usage in the subscription for
'project-json-changed'events.
41-51: Read definition logic looks straightforward.Fetching the service through
ctx.getApi(id)and returningcontentsis clear and matches the code’s intent.
53-66: Subscription approach is appropriate.Using an observable to emit updates when
'project-json-changed'triggers is a clean solution. No issues found.
68-80: Write definition mutation is properly structured.Calling
api.writeDefinitionwith the new contents and old hash is consistent with typical optimistic concurrency checks. No issues found.packages/project-builder-lib/src/migrations/index.ts (2)
28-40: Custom error class is well-structured.Including the migration name and the underlying cause is helpful for debugging and logging. No further issues found.
43-65: Renaming to "migratedDefinition" clarifies intent.Renaming
newConfigtomigratedDefinitionimproves readability. The try/catch around each migration call and the customSchemaMigrationErrorfacilitate more robust error handling.packages/project-builder-web/src/app/ClientVersionProvider/ClientVersionProvider.tsx (4)
5-12: Imports look good.These imports from various internal services, hooks, and error logging are well-aligned with their usage in the subsequent code. Good job keeping everything in one place.
14-17: Interface definition is concise and clear.Defining just a
childrenprop is sufficient for this provider. This approach keeps the component flexible while clearly communicating usage.
18-20: Provider function signature is well-defined.Renaming to
ClientVersionProviderand accepting the new props matches your new approach for version management.
37-43: WebSocket reconnection logic looks good.Attaching the
fetchVersioncall to the'open'event ensures the version is refreshed automatically after re-establishing the connection, and returning the unsubscribe callback properly cleans up the listener.packages/project-builder-web/src/pages/data/models/edit/[id]/_components/ModelHeaderBar.tsx (4)
6-6: Revised imports help maintain consistency with UI patterns.Using
ButtonanduseConfirmDialogfrom the same UI suite keeps interactions consistent across your application.
22-23: Destructuring fromuseProjectDefinitionis clear.Extracting
definition,saveDefinitionWithFeedbackSync, andisSavingDefinitionclarifies which functionalities your component depends on.
28-37: Deletion flow is streamlined.Shifting to
saveDefinitionWithFeedbackSyncwith an inline draft updater and success callback simplifies error handling and ensures consistent feedback. This is a clean, unified approach.
69-69: Disabling the delete button during saves is a good UX practice.Preventing multiple concurrent actions helps avoid race conditions or partial updates.
packages/project-builder-web/src/app/ProjectDefinitionProvider/services/project-definition-file-manager.ts (7)
1-2: Utility import usage is appropriate.Importing
hashWithSHA256from a shared utility is straightforward and helps ensure a consistent hashing approach across the codebase.
3-7: API imports align with manager responsibilities.Downloading, uploading, and listening for definition file changes belong logically in this manager, keeping network concerns encapsulated.
9-13: Interface definition is straightforward.Exposing
contents,hash, andupdatedExternallyis sufficient for client usage. Good job limiting the interface to essentials.
15-23: Class design is modular and clear.Encapsulating file logic in
ProjectDefinitionFileManagerfosters separation of concerns. TheprojectIdis well-scoped to an instance.
25-41: Download logic is concise with correct hashing checks.Your
updatedExternallycondition helps detect external modifications reliably. Consider elaborating concurrency strategies if external writes can happen during multiple downloads.
42-87: Upload logic handles concurrency well.Throwing an error if there's already a pending upload clarifies usage. The final block resets
_pendingDefinitionFileContentsHashfor re-entrancy. This is a solid approach to avoid overwriting in-flight changes.
89-108: Change listener updates local hash and calls back.This method effectively detects remote updates while respecting the local
_definitionFileContentsHash. Great pattern for pushing updates to the consumer.packages/project-builder-web/src/pages/apps/edit.page.tsx (5)
6-6: No concerns on this import update.
The addition ofButtonandDialogreferences looks straightforward and aligns with the UI changes below.
16-16: Destructure usage updated successfully.
Swapping out legacy references in favor ofsaveDefinitionWithFeedbackSync,definition, andisSavingDefinitionis a clean approach to unify the control flow. Ensure all previous references toparsedProjectandsetConfigAndFixReferencesare removed from dependent components.
28-40: Enhanced delete handling logic is well-structured.
- Using
saveDefinitionWithFeedbackSyncwith a callback filtering the deleted app fromdefinition.appsis both succinct and clear.- The
optionsobject providingsuccessMessageand anonSuccesscallback simplifies post-deletion flow.Overall, this block is neatly handling user feedback and navigation. Just ensure any references to the removed app are also pruned if necessary in upstream code.
43-43: AccessingpackageScopefromdefinition
This is consistent with the new state shape. Make sure other references todefinition's fields are updated consistently to avoid undefined or stale state.
74-78: Disable button during save is a user-friendly improvement.
Disabling the destructive button whenisSavingDefinitionis true prevents duplicate deletions and race conditions.packages/project-builder-lib/src/references/fix-ref-deletions.ts (6)
6-10: New type imports are aligned with the updated reference-handling approach.
Bringing inResolvedZodRefPayloadconsolidates type safety and clarity in the reference flow.
12-12: ImportingparseSchemaWithReferences.
This is a logical and well-named function to parse the schema thoroughly. Make sure that all references to legacy parse calls are removed if not needed.
17-17: ReturningrefPayloadin success results.
Including the parsed reference payload in the result object improves post-processing, especially when further reference manipulations might be needed. This is an excellent improvement in the function's output structure.
46-49: Initializing iteration count andvalueToEdit.
- The
forloop approach is valid to mitigate infinite recursion when references are scattered.- Consider if an exponential backoff or bail-out condition might be needed beyond the iteration cap to detect cyclical references.
73-93: Handling missing array references viatryDeleteParent.
- Checking if the parent is an array and marking entries for deletion is a neat approach.
- The code ensures array indexes are tracked in reverse order to avoid index shifting.
- Confirm that the sentinel
'deleted-sentinel-id'is never incorrectly assigned in normal usage, so we don’t accidentally skip legitimate references.
95-145: Comprehensive fix flow with repeated parse attempts.
- The function gracefully addresses leftover references with each iteration.
- Returning a failure result once the iteration ends or issues remain is robust.
- The final
throw new Error(...)for exceeding max iterations helps avoid infinite loops—good safeguard.packages/project-builder-web/src/app/ProjectDefinitionProvider/ProjectDefinitionProvider.tsx (16)
1-7: Importing new types for type-safe project definition management.
Utilizing these definitions ensures better clarity when passing around setter and feedback options.
8-13: Importing reference-fixing utilities and context.
Centralizing logic inproject-builder-libfosters consistency. Good usage of typed references to unify schema parsing.
16-20: UI components import block.
Looks correct, withtoastproviding an easy way to show messages—standard for user feedback.
21-21: Immer-based immutability usage.
Theproducefunction fromimmeris a clean approach for atomic state updates.
22-23: Initialization of local states and derived values.
UsinguseStateforisSavingDefinitionanduseMemoforresultis an excellent move for performance. Just ensure that any derived states are invalidated on relevant dependencies.
29-29:useDeleteReferenceDialog
This custom hook presumably surfaces a UI flow for reference deletion issues. It’s consistent with your broader reference deletion logic.
31-38: ProjectDefinitionProviderProps interface
The optionalchildrenproperty ensures flexibility in usage of the provider.
40-58: Provider definition: main logic initialization
- The code properly fetches, parses, and sets up the environment for the project definition.
- The approach ensures that errors from multiple sources (
contextError,definitionError,containerError) are captured.
59-62:updatedExternallystate
A helpful signal to detect changes that happen outside this flow. Consider how it’s consumed in the rest of the app to avoid overwriting external modifications.
63-65: useMemo block check
Exiting early if noprojectDefinitionContainerorschemaParserContextis found is a good safety measure.
66-105:saveDefinitionfunction
- Using
immerto prepare a new project definition is clean and predictable.fixRefDeletionsis called to ensure references are valid after mutation.- Properly reassigning
cliVersionkeeps versioning consistent.- The
try/finallyensures loading state resets in all outcomes. Nicely done.
107-136:saveDefinitionWithFeedback
- Handling user notifications (success, error, or reference issues) is well abstracted.
- The optional
disableDeleteRefDialogflag prevents unwanted UI prompts when desired.- Returning a success object after
.then()is a simple way to chain calls.
137-148: ExposingsaveDefinitionWithFeedbackSync
- Wrapping
saveDefinitionWithFeedbackin a sync-like call is a neat approach for event-based triggers (like button onClicks).- Consider also returning the promise or an indication in case the caller needs further hooking.
[architectural_advice]
159-205: Error and loading states
- The
ErrorableLoaderusage is straightforward, showing fallback UIs if something fails.- The approach of switching the project is user-friendly.
208-247: CLI version conflict check
- Validating
cliVersionandsemver.gtis a good approach to ensure the user has the right environment.- The fallback explanatory UI with an option to switch projects is a smart user experience design.
249-253: Final provider structure
WrappingchildreninProjectDefinitionContext.Providerensures broad reusability for everything inside your application.packages/project-builder-server/src/service/builder-service.ts (7)
19-19: Usage of promises-based file I/O is good.
Switching toreadFile/writeFilefrom'node:fs/promises'is a clean, modern approach.
42-54: Write result union type is well-structured.
This union type clearly differentiates successful writes from content mismatch errors. As a future enhancement, you might consider adding more error/result states, such as filesystem permission issues.
70-73: Typed event interface helps reduce runtime errors.
Explicitly defining'project-json-changed'and'command-console-emitted'events promotes clarity and maintainability.
75-75: Leverage generics in TypedEventEmitter.
UsingTypedEventEmitter<ProjectBuilderServiceEvents>ensures event parameters are strongly typed, preventing accidental misusage of event payloads.
128-136: Sensible approach to JSON file changes.
Reading the latest definition and emitting'project-json-changed'keeps the file watcher reactive and consistent. Error logging is appropriate.
148-148: Watcher initialization is straightforward.
Chokidar is employed effectively here. If performance becomes a concern in the future, consider more granular watchers or debouncing.
171-185: Read method is concise and robust.
Handling a missing file withhandleFileNotFoundErrorand falling back to_getInitialProjectDefinitionis a neat approach. Thread-safe concerns appear minimal here, but keep concurrency in mind if you expand behavior.packages/project-builder-web/src/pages/features/auth/index.tsx (11)
5-5: Great use of model patching utilities in imports.
IntroducingapplyModelPatchInPlace,diffModel, andModelUtilshelps unify model transformations. This is likely to reduce boilerplate elsewhere.Also applies to: 9-10
16-16: UiAlert import is beneficial.
Wrapping warnings or notifications in a dedicated alert component can streamline user messaging.
18-18: Cleanly separated usage of React hooks and local components.
UsinguseState,useMemo, and a customButtonkeeps the code modular.Also applies to: 19-19
23-23: Refactor for dedicated model generation.
createAuth0Modelscentralizes Auth0-specific model creation, promoting Single Responsibility and maintainability.
27-32: Extend project definition for sync and feedback.
DestructuringdefinitionContainer,saveDefinitionWithFeedback, andsaveDefinitionWithFeedbackSyncis a clear approach for flexible save operations.
43-45: Reading model references from definition.
StoringuserModelRefanduserRoleModelRefhelps ensure that the correct models are leveraged for reference resolution.
63-86: onSubmit patch logic is well-organized.
Applying the patches to thedraftConfigensures an atomic update. Good job utilizingpendingModelChangescheck.
102-112: Graceful disableAuth flow.
saveDefinitionWithFeedbackSyncplus anonSuccesshandler is succinct and user-friendly, clearing out local form states. Make sure to log or handle any potential errors.
129-131: Form submission wiring is consistent.
onSubmitreferences are properly bound, and thedisableAuthbutton is placed logically.
132-166: Detailed user model change alerts.
Presenting the number of fields, relations, and constraints to be updated improves transparency. This is user-friendly.
167-202: Detailed user role model change alerts.
Similar to user model changes, providing breakdown fosters clarity for the user. Make sure to keep the UI consistent if additional role model changes are introduced.packages/project-builder-lib/src/tools/model-merger/model-merger.ts (12)
1-2: Useful utility import for structural comparison.
isEqualfromes-toolkitis a simple, effective way to handle deep comparison of objects.
3-9: Separate definition and schema references.
ImportingProjectDefinitionContainerand various configs cleanly clarifies dependencies. Good practice.
11-19: Helper imports for ID generation and references.
Leveraging theModelUtilsand typed entity references will reduce manual overhead when generating new IDs or referencing models.
22-27: DiffOperation interface is concise.
Providing a clear enum-like'add' | 'update' | 'remove'with a key makes patch application straightforward.
28-41: Straightforward scalar, relation, and constraint types.
Omitting theidfields helps ensure that new items can be assigned IDs on creation.
43-51: ModelDefinitionInput shape.
Defining an input structure for fields, relations, and constraints fosters a robust pipeline for diffing and patching.
53-61: ModelDiffOutput enumerates all possible changes.
Capturing added/updated/removed fields, relations, constraints, and primary key changes sets a good foundation for consistent merges.
63-66: Optional removals with enableRemove.
This makes the diff function flexible, accommodating scenarios where removal is or is not desired.
76-106: diffByKey function is well-structured.
Iterating over desired items first to handle adds/updates, then optionally removing unneeded items, is intuitive. The optionalenableRemovelogic is beneficial.
108-150: applyPatchByKey with ID preservation strategy.
Retaining the existing ID upon update is critical for stable references; throwing an error on missing items for update avoids silent failures.
152-163: Constructing a unique key for constraints.
Sorting field refs ensures consistent key generation. This approach prevents random ordering from generating spurious diffs.
233-309: applyModelPatchInPlace uses a well-defined pipeline.
The final reference resolution logic is especially important to ensure local and foreign refs remain valid. This approach is aligned with standard migration patterns.packages/project-builder-lib/src/references/ref-builder.ts (10)
23-25: Doc comment addition looks good.
The concise description forzRefIdhelps clarify its intended usage.
46-49: Doc comment is clear and consistent.
No issues found with the explanation of the "resolve name" mechanism.
69-80: Helper function usage.
createDefinitionEntityNameResolversimply returns the passed resolver, which is fine. No issues found.
82-84: Base entity definition changes are well-documented.
The inclusion ofgetNameResolveradds flexibility. No issues found.Also applies to: 85-106
113-114: Doc comments for entity/reference interfaces and builder classes.
All added doc comments increase readability. No concerns or corrections needed.Also applies to: 125-126, 137-139, 151-152, 178-180, 189-190, 191-194, 200-203
209-219: Constructor and property definitions.
The new class fields (references,entitiesWithNameResolver, etc.) are coherent. No issues found with state initialization.Also applies to: 220-225, 232-232
239-245: Path construction methods and documentation.
The doc comments explain the reference path logic clearly. Implementation appears logically sound.Also applies to: 256-261, 268-277
301-355:addReferencemethod logic.
Null/undefined check for reference value is a good safeguard. The parent path checks are correct. No issues found.
357-375:addEntitymethod enhancements.
Using a dynamic name resolver is a great extension for entity management. Ensure you handle the case where agetNameResolvermight return an invalid or empty string.Also applies to: 381-381, 389-389, 392-392, 393-395, 398-398, 414-422, 423-423
433-451: Bulk changes: ZodRef, ZodRefWrapper, and associated utilities.
These extensive refactors appear consistent, with robust doc comments and correct usage of shared contexts. The layering of parse steps and transformations is well-structured.Also applies to: 469-471, 474-474, 485-488, 491-491, 501-502, 533-606, 610-617, 619-665, 667-681, 683-748, 751-759, 761-772, 773-846
packages/project-builder-web/src/services/config.ts (1)
12-12: NewIS_PREVIEWflag.
ExportingIS_PREVIEWfrom config is a straightforward approach. No issues found.packages/project-builder-web/src/services/api/version.ts (1)
1-14: Preview-mode version handling.
Returning a hardcoded preview response vs. queryingtrpc.version.get.query()aligns well with your preview toggle. Implementation looks consistent.packages/project-builder-server/src/api/plugins.ts (1)
5-17: LGTM! Clean router implementation.The router definition is well-structured, with proper input validation and clear separation of concerns. The mutation handler is concise and follows the single responsibility principle.
packages/project-builder-server/src/api/version.ts (2)
5-8: LGTM! Well-defined interface.The
ClientVersionInfointerface is clear and follows TypeScript best practices.
10-17: LGTM! Clean and type-safe router implementation.The router is well-implemented with proper type annotations and follows tRPC patterns.
packages/project-builder-web/src/services/api/projects.ts (1)
6-22: LGTM! Clean implementation with proper preview mode handling.The function is well-documented and handles both preview and non-preview modes appropriately, with type-safe mock data.
packages/project-builder-server/src/api/index.ts (1)
2-19: LGTM! Clean router configuration.The router configuration is well-organized, with clear separation of concerns across different routers. The type exports are properly defined, making the API structure clear and maintainable.
packages/project-builder-web/src/services/error-formatter.ts (1)
9-9: LGTM! More actionable error message.The new error message provides better guidance by directing users to check console logs for detailed information, which is more helpful for debugging than the previous generic message.
packages/project-builder-web/src/services/hot-loader.ts (1)
1-23: LGTM! Well-implemented hot reload subscription.The implementation is robust with:
- Type-safe event handling using generics
- Proper cleanup through unsubscribe function
- Graceful handling of environments without hot reload support
- Clear documentation
packages/project-builder-web/src/services/api/plugins.ts (1)
1-22: LGTM! Well-structured plugin metadata retrieval.The implementation is solid with:
- Clear handling of preview mode
- Proper error handling for project not found cases
- Well-documented API with JSDoc
- Type-safe mutation call
packages/project-builder-server/src/compiler/backend/enums.ts (3)
3-3: LGTM!The import of
ProjectDefinitiontype aligns with the PR's objective of refactoring project definition handling.
20-20: LGTM!The parameter type change from
ParsedProjectDefinitiontoProjectDefinitionis consistent with the refactoring objectives.
22-24: LGTM!The changes improve null safety by using the nullish coalescing operator while maintaining the same filtering logic.
packages/project-builder-lib/src/references/parse-schema-with-references.ts (1)
1-9: LGTM!The imports are well-organized and include all necessary types and functions.
packages/project-builder-server/src/api/sync.ts (1)
8-14: LGTM!The router definition and input schema are well-structured and type-safe.
packages/project-builder-web/src/app/ProjectSelectorGate/ProjectSelectorGate.tsx (3)
8-8: LGTM! Import path updated.The import path for
getProjectshas been updated fromsrc/services/remoteto@src/services/api, which aligns with the broader refactoring of the project's service layer.
11-11: LGTM! Component and interface renamed for better clarity.The renaming from
ProjectChoosertoProjectSelectorin both the component and interface names provides better clarity about the component's purpose and maintains consistency across the codebase.Also applies to: 13-15, 20-22
17-19: LGTM! Added descriptive JSDoc comment.The added JSDoc comment clearly describes the component's purpose: "Gate that ensures a project is selected before rendering the app."
packages/project-builder-web/src/pages/features/auth/auth-models.ts (1)
6-21: LGTM! Well-defined Auth0 user model fields.The
AUTH0_MODEL_FIELDSarray provides a clear definition of user model fields with appropriate types and options:
- UUID generation for the
idfield- Required
- Optional
auth0Idfield for external authenticationpackages/project-builder-lib/src/references/serialize-schema.ts (2)
10-12: LGTM! Enhanced type safety with ResolvedZodRefPayload.The function signature now uses
ResolvedZodRefPayloadinstead ofZodRefPayload, which better represents the resolved state of references in the payload.
43-43: LGTM! Improved schema parsing mechanism.Replaced the old
ZodRefWrapper.create(schema).parse(value)with the newparseSchemaWithReferences(schema, value)function, which provides a more direct and maintainable approach to parsing schemas with references.packages/project-builder-lib/src/web/hooks/useResettableForm.ts (2)
19-19: LGTM! Simplified external change tracking.Replaced the numeric counter with a boolean flag
updatedExternally, which provides a clearer and more maintainable way to track external changes.Also applies to: 31-31
43-43: LGTM! Updated effect dependencies.The effect's dependency array has been correctly updated to include
updatedExternallyandisDirty, ensuring proper reactivity to external changes and form state.packages/project-builder-web/src/hooks/useProjects.ts (1)
1-1: LGTM! Type updates improve consistency.The change from local
Projecttype to server-sideProjectInfotype aligns with the refactoring objectives and improves type consistency across packages.Also applies to: 8-8, 10-10
packages/project-builder-web/src/pages/data/models/edit/[id]/graphql.page.tsx (1)
20-22: LGTM! Improved code clarity.Destructuring form properties and passing specific values to
useBlockUnsavedChangesNavigatemakes the dependencies clearer and improves code maintainability.packages/project-builder-web/src/app/ProjectDefinitionProvider/services/parse-project-definition-contents.ts (1)
13-49: LGTM! Well-structured error handling.The function implements a robust parsing flow with proper error handling for various failure cases. The separation of concerns between parsing, migration, and validation is clear and maintainable.
packages/project-builder-web/src/app/ProjectDefinitionProvider/services/schema-parser-context-manager.ts (1)
11-49: LGTM! Well-designed context management.The
SchemaParserContextManagerclass provides a clean and maintainable way to handle schema parser context with proper lifecycle management and hot reload support.packages/project-builder-lib/src/plugins/schema/parser.ts (1)
53-55: LGTM! Clean implementation of the pluginStore getter.The getter provides a clean and type-safe way to access the pluginStore property while maintaining encapsulation.
packages/project-builder-web/src/app/App.tsx (1)
15-17: LGTM! Well-structured component hierarchy with improved state management.The refactoring from gate components to providers enhances the state management approach while maintaining a clear separation of concerns. The component tree is well-organized with:
- Error handling at the top level
- Version management through
ClientVersionProvider- Project selection through
ProjectSelectorGate- Project definition management through
ProjectDefinitionProvider- UI components (
Toaster,ConfirmDialog,BlockerDialog) properly placed as direct children ofTooltip.ProviderAlso applies to: 32-43
packages/project-builder-web/src/app/ProjectDefinitionProvider/hooks/use-schema-parser-context.ts (2)
11-14: LGTM! Well-defined hook interface with proper state management.The hook's interface and state management follow React best practices:
- Clear interface definition with
UseSchemaParserContextResult- Proper state initialization for both context and error
Also applies to: 21-27
28-54: LGTM! Comprehensive effect handling with proper cleanup.The effect implementation is robust:
- Proper cleanup of state on dependency changes
- Early return for invalid conditions
- Error handling for both initial load and plugin asset changes
- Cleanup function returns the listener unsubscribe
packages/project-builder-web/src/pages/apps/list.page.tsx (1)
11-13: LGTM! Simplified data access pattern.The changes streamline the data access by directly using the
definitionobject, which is consistent with the broader refactoring effort in the codebase.packages/project-builder-web/src/app/ProjectDefinitionProvider/hooks/use-project-definition-container.ts (1)
11-19: LGTM! Well-structured interface definitions.The interfaces are clear, properly typed, and follow TypeScript best practices by explicitly marking optional values using undefined union types.
packages/project-builder-web/src/services/api/project-definition.ts (2)
43-60: LGTM! Robust content hash verification.The hash verification prevents concurrent modification conflicts, enhancing data integrity.
18-33:Details
❓ Verification inconclusive
Verify preview mode security implications.
The preview mode fetches from a static JSON file. Ensure this doesn't expose sensitive data in production builds.
🏁 Script executed:
#!/bin/bash # Check if preview-app.json exists in production builds find . -name "preview-app.json" -type f -exec grep -l "sensitive" {} \;Length of output: 72
Action: Manually verify preview mode file safety
The function correctly fetches the static JSON file when
IS_PREVIEWis enabled. However, our initial grep check (searching for the literal word "sensitive") did not yield any output. This result is inconclusive regarding whether the file might contain other sensitive data. Please manually verify that thepreview-app.jsoncontent—especially in production builds—is appropriate and does not inadvertently expose any sensitive information.
- Confirm that
preview-app.jsonis only deployed with preview builds.- Double-check the file’s contents for any sensitive data that might not be caught by a simple keyword search.
packages/project-builder-server/src/compiler/backend/feature.ts (1)
25-41: LGTM! Clean removal of parsedProject dependency.The refactor simplifies the data flow by using projectDefinition directly, aligning with the PR's objectives.
packages/project-builder-web/src/components/Console/index.tsx (1)
14-14: LGTM! Clean migration to tRPC.The changes correctly migrate from the legacy API client to tRPC while maintaining the same functionality.
Also applies to: 44-44
packages/project-builder-lib/src/web/hooks/useProjectDefinition.ts (2)
8-14: Well-documented type definition.The
ProjectDefinitionSettertype is well-documented and clearly explains its purpose and behavior with Immer.
25-70: LGTM! Comprehensive interface with good documentation.The
UseProjectDefinitionResultinterface is well-structured with clear JSDoc comments for each property. The new save methods with feedback enhance user experience.packages/project-builder-server/src/compiler/admin/crud/displays.ts (1)
22-23: LGTM! Consistent use of ModelUtils.The changes correctly migrate from using
parsedProject.getModelByIdtoModelUtils.byIdOrThrow, maintaining consistent error handling across the codebase.Also applies to: 54-55
packages/project-builder-web/src/app/ProjectDefinitionProvider/NewProjectCard.tsx (2)
21-21: LGTM! Improved error handling and async flow.The changes correctly handle asynchronous project saving with proper error feedback using toast notifications.
Also applies to: 40-44
28-32: LGTM! Added loading state handling.The component now correctly handles loading state during form submission, preventing multiple submissions.
Also applies to: 75-75
packages/project-builder-web/src/pages/data/models/edit/[id]/service.page.tsx (1)
27-29: LGTM! Form state management improvements.The changes enhance form state handling by adding reset capability and simplifying the unsaved changes navigation hook parameters, making the code more maintainable.
packages/project-builder-web/src/pages/data/enums/edit/components/EnumHeaderBar.tsx (1)
22-23: LGTM! Enhanced save handling and UX.The changes improve user experience by:
- Preventing multiple submissions during save operations
- Using a consistent approach to saving definitions with feedback
- Proper handling of navigation after successful deletion
Also applies to: 27-38, 79-79
packages/project-builder-web/src/app/ProjectSelectorGate/ProjectSelectorDialog.tsx (2)
14-24: LGTM! Clear and consistent naming improvements.The renaming from "Chooser" to "Select" and the import path reorganization improve code clarity and maintainability.
48-51: LGTM! Better comment clarity.Changed from "dummy" to "no-op" which better describes the function's purpose.
packages/project-builder-web/src/app/AppLayout/ProjectSyncModal.tsx (2)
13-13: LGTM! Simplified API dependencies.The removal of FilePayload and lastModifiedAt aligns with the PR's objective to streamline the sync flow.
Also applies to: 25-26
32-43:Details
❓ Verification inconclusive
Verify sync behavior with simplified payload.
The sync flow has been simplified by removing the payload construction. Please ensure this doesn't affect sync functionality.
🏁 Script executed:
#!/bin/bash # Check for any existing sync-related tests rg -l "startSync.*test"Length of output: 25
SYNC FUNCTION VERIFICATION: Simplified Payload
The sync flow’s payload construction was removed, so please ensure that the removal does not impact the intended sync behavior. The automated search for sync-related tests (using the pattern
"startSync.*test") yielded no results, which means we couldn’t automatically verify this change. It would be best to perform manual regression exercises focused on sync functionality.
- Action Items:
- Verify that the sync operation (i.e.
startSync(currentProjectId)) functions as expected without the previously constructed payload.- Review and, if necessary, enhance test coverage for sync behavior to capture potential regressions.
- Confirm that any expectations tied to the former payload are no longer applicable.
packages/project-builder-web/src/pages/apps/edit/BackendAppForm.tsx (3)
21-22: LGTM! Improved state management with feedback.The use of saveDefinitionWithFeedback and isSavingDefinition provides better user feedback during save operations.
30-36: LGTM! Simplified form submission logic.The form submission logic is now more straightforward and uses the new saveDefinitionWithFeedback pattern consistently.
88-90: LGTM! Better UX during save operations.Disabling the save button while saving prevents duplicate submissions and provides clear visual feedback.
packages/project-builder-lib/src/compiler/app-compiler-spec.ts (1)
20-22:Details
❓ Verification inconclusive
Verify the impact of return type changes.
The return type now allows for both single and array of GeneratorBundle. Please ensure all consumers handle both cases correctly.
🏁 Script executed:
#!/bin/bash # Find usages of getChildrenForFeature rg "getChildrenForFeature" -A 3Length of output: 1167
Ensure proper handling of flexible return types in consumer code.
The update to the
getChildrenForFeaturereturn type now allows individual properties to be either a singleGeneratorBundleor an array of them. Please verify that all consumers—especially inpackages/project-builder-server/src/compiler/backend/feature.ts—properly handle both cases. Double-check that any downstream operations or type checks accommodate this dual possibility, and consider normalizing the output if a consistent type is expected.packages/project-builder-web/src/app/ProjectDefinitionProvider/hooks/use-remote-project-definition-contents.ts (2)
10-14: LGTM! Well-structured interface definition.The interface clearly defines the hook's return value with appropriate types.
16-24: LGTM! Clean state initialization.Good practice using multiple useState hooks for separate concerns and destructuring useProjects hook values.
packages/project-builder-web/src/pages/apps/edit/WebAppForm.tsx (1)
21-30: LGTM! Clean hook usage and form setup.Good practice using useResettableForm with zodResolver for validation.
packages/project-builder-server/src/compiler/backend/fastify.ts (2)
40-41: LGTM! Simplified dependency access.Good refactoring to access projectDefinition and appCompiler directly from the builder.
102-102:Details
❓ Verification inconclusive
Verify the impact of getRootChildren changes.
The removal of parsedProject.fastifyChildren in favor of appCompiler.getRootChildren() might affect the structure of generated bundles.
🏁 Script executed:
#!/bin/bash # Search for any custom implementations of getRootChildren ast-grep --pattern 'getRootChildren() { $$$ }'Length of output: 50
Action Required: Manually verify the impact of the changes to getRootChildren
The initial AST search did not reveal any custom implementations of
getRootChildren, but the absence of evidence here is not conclusive. Please run a manual review of the implementation details forgetRootChildrento ensure that replacingparsedProject.fastifyChildrenwithappCompiler.getRootChildren()does not inadvertently affect the structure of the generated bundles.
- File to review:
packages/project-builder-server/src/compiler/backend/fastify.ts(specifically around line 102)- Next steps:
- Confirm that
appCompiler.getRootChildren()returns the correct structure needed for bundling.- Verify if there are any overlooked custom overrides or usage patterns in the codebase that could be impacted by this change.
packages/project-builder-lib/src/references/deserialize-schema.ts (1)
7-9: LGTM! Clean type imports.Good practice importing types explicitly and using .js extension for imports.
packages/project-builder-web/src/app/ClientVersionProvider/ClientVersionProvider.test.tsx (2)
13-131: LGTM! Comprehensive test coverage with well-structured test cases.The test suite effectively covers all critical aspects of the ClientVersionProvider component:
- Version info fetching
- Loading states
- Error handling
- Version change detection
- Context provision
21-41: LGTM! Clean mock setup and teardown.The beforeEach and afterEach hooks properly handle:
- Mock initialization
- WebSocket event cleanup
- Global location object restoration
packages/project-builder-lib/src/references/types.ts (3)
5-15: LGTM! Well-documented class and methods.The added JSDoc comments provide clear and comprehensive documentation for the DefinitionEntityType class and its methods.
Also applies to: 24-28, 33-38
57-92: LGTM! Clear and well-structured interface definitions.The DefinitionEntity and DefinitionReference interfaces are well-documented and provide a robust structure for type safety.
Also applies to: 112-132
157-175: LGTM! Type-safe payload interface.The ResolvedZodRefPayload interface provides a clear structure for handling resolved entity names with proper typing.
packages/project-builder-web/src/pages/settings/components/FeatureForm.tsx (2)
65-114: LGTM! Improved form submission with proper feedback handling.The refactored submission logic using saveDefinitionWithFeedback provides better control over the success flow and error handling.
162-164: LGTM! Proper submit button state management.The submit button is correctly disabled during form submission, preventing duplicate submissions.
packages/project-builder-web/src/pages/data/enums/hooks/useEnumForm.tsx (3)
90-107: LGTM! Improved duplicate name validation.The code now properly validates enum names case-insensitively before saving, preventing duplicate names and providing clear error feedback.
109-141: LGTM! Enhanced save operation with feedback.The new implementation using
saveDefinitionWithFeedbackprovides:
- Clear success messages
- Proper state management during save operations
- Consistent error handling
157-157: LGTM! Added loading state indicator.The addition of
isSavingDefinitionto the return value allows components to show loading states during save operations.packages/project-builder-web/src/pages/apps/edit/admin/crud/AdminCrudEmbeddedForm.tsx (1)
95-106: LGTM! Simplified model options retrieval.The change from
parsedProject.getModels()to directly accessingdefinition.modelssimplifies the code and aligns with the project's refactoring goals.packages/project-builder-lib/src/parser/parser.ts (2)
12-12: LGTM! Enhanced type safety with ResolvedZodRefPayload.The migration from
ZodRefPayloadtoResolvedZodRefPayloadimproves type safety for reference handling.Also applies to: 126-126
133-133: LGTM! Improved schema parsing with references.The switch to
parseSchemaWithReferencesprovides a more robust way to handle schema references.packages/project-builder-web/src/pages/plugins/PluginCard.tsx (3)
50-69: LGTM! Enhanced plugin enabling with feedback.The new implementation using
saveDefinitionWithFeedbackSyncprovides immediate feedback and prevents race conditions during plugin enabling.
73-82: LGTM! Enhanced plugin disabling with feedback.Similar improvements to the disable operation, providing clear feedback and preventing race conditions.
117-123: LGTM! Added loading state to prevent double submissions.The buttons are now properly disabled during save operations, preventing potential race conditions.
Also applies to: 133-139
packages/project-builder-lib/src/definition/project-definition-container.ts (2)
9-9: LGTM! Type safety improvement.The change from
ZodRefPayloadtoResolvedZodRefPayloadimproves type safety by ensuring that references are resolved before being used.Also applies to: 34-34, 41-41
106-109: LGTM! Enhanced serialization.The change to use
prettyStableStringifyensures consistent and readable serialization output.packages/project-builder-server/src/server/plugin.ts (2)
61-67: LGTM! Improved context creation.The change to use
createContextBuilderwith a structured configuration object improves code clarity and maintainability.
46-47:Details
❓ Verification inconclusive
Verify service initialization timing.
The removal of
awaitbeforeservice.init()could lead to race conditions if the initialization process includes asynchronous operations that need to complete before the service is used.
🏁 Script executed:
#!/bin/bash # Search for async operations in service.init() ast-grep --pattern 'init() { $$$ await $$$ $$$ }'Length of output: 57
Service Initialization Timing Check
The recent search for any
awaitstatements withinservice.init()returned no evidence of asynchronous operations. However, since the initialization was previously awaited, please manually verify that no asynchronous logic is performed withinservice.init(). If asynchronous operations are present in the initialization process, the omission ofawaitmight lead to race conditions.
- File: packages/project-builder-server/src/server/plugin.ts (lines 46-47)
- Action Required: Confirm that
service.init()does not perform asynchronous operations that require completion before subsequent service usage. If such operations exist, reintroduce theawaitto ensure correct service initialization timing.packages/project-builder-server/src/compiler/backend/services.ts (1)
3-3: LGTM! Type refinement.The change from
ParsedModeltoModelConfigaligns with the broader refactor to use more specific and descriptive types.Also applies to: 73-73, 93-93
packages/project-builder-web/src/pages/apps/edit/admin/AdminEditSectionForm.tsx (2)
41-47: LGTM! Enhanced state management.The destructured hooks provide better control over the saving process and UI feedback.
183-185: LGTM! Improved button state.The disabled state during saving prevents duplicate submissions and provides clear feedback to users.
packages/project-builder-lib/src/references/resolve-zod-ref-payload-names.unit.test.ts (1)
1-227: Great test coverage!The test suite is well-structured and comprehensive, covering essential scenarios:
- Simple entity names without dependencies
- Entity names with dependencies in correct order
- Array dependencies
- Error handling for unresolvable dependencies
- Invalid references handling
- Reference name resolution skipping
- Input data preservation
packages/project-builder-lib/src/schema/models/index.ts (1)
147-147: LGTM! Improved name resolution.The change from static
namePathto dynamicgetNameResolveraligns with the PR objectives and provides more flexibility in resolving foreign relation names.packages/project-builder-lib/src/references/ref-builder.unit.test.ts (1)
1-225: Excellent test coverage!The test suite is well-structured and comprehensive, covering essential scenarios:
- Simple schema parsing
- Entity collection and reference resolution
- Nested and parent references
- Chaining of references
- Custom name resolvers
- Name resolvers with dependencies
The tests are well-documented with clear descriptions and comments explaining the purpose of each test case.
packages/project-builder-web/src/pages/settings/theme-builder.tsx (1)
27-44: LGTM! Improved save flow.The changes align well with the PR objectives:
- Replaced
setConfigAndFixReferenceswithsaveDefinitionWithFeedback- Simplified error handling by leveraging the new feedback mechanism
- Improved code clarity by using more descriptive variable names
packages/project-builder-web/package.json (1)
44-50:Details
✅ Verification successful
LGTM! Testing dependencies added.
The addition of testing libraries and utilities enhances the project's testing capabilities. The fixed versions ensure reproducibility.
Let's verify if these are the latest stable versions of the testing libraries:
Also applies to: 62-62
🌐 Web query:
What are the latest versions of @testing-library/react, @testing-library/dom, @testing-library/jest-dom, and @testing-library/user-event?💡 Result:
Here are the latest versions of key Testing Library packages based on available information as of February 2025:
Core Libraries
1️⃣ @testing-library/react
Latest: v16.2.0 (Released 2025-01-15)
Key changes in recent major versions:
- Requires React 18+ and React DOM 18+[6]
- Now uses
@testing-library/domas peer dependency[6]2️⃣ @testing-library/dom
Latest: v10.4.0 (Released 2024-07-22)
Recent updates include:
- Dropped Node.js <18 support[7]
- Updated ARIA role handling[7]
Utilities
3️⃣ @testing-library/jest-dom
Latest: v6.6.3 (Released October 2024)
Key features:
- Extended Jest/Vitest matchers like
.toBeVisible()and.toBeDisabled()[3][8]- Supports both Jest and Vitest via separate imports[8]
4️⃣ @testing-library/user-event
Current recommendation: v14+
Key updates since v13:
- Improved async interaction handling[9]
- Requires explicit setup with
userEvent.setup()[9]- Better browser interaction simulations than
fireEvent[9]Migration Notes
# Recommended installation command for new projects: npm install --save-dev @testing-library/react@16 @testing-library/dom@10 @testing-library/jest-dom@6 @testing-library/user-event@14Important compatibility notes:
• React Testing Library v16 requires React 18+/DOM 18+[6][10]
• User Event v14 fixes async interaction warnings present in v13[5][9]
• Jest DOM works with both Jest and Vitest via separate import paths[8](Version data sourced from official release notes and package registries)[1][3][6][7][9]
Citations:
- 1: https://github.com/callstack/react-native-testing-library/releases
- 2: https://classic.yarnpkg.com/en/package/dom-testing-library
- 3: https://www.npmjs.com/package/@testing-library/jest-dom
- 4: https://testing-library.com/docs/user-event/v13/
- 5: testing-library/user-event#1104
- 6: https://github.com/testing-library/react-testing-library/releases
- 7: https://github.com/testing-library/dom-testing-library/releases
- 8: https://markus.oberlehner.net/blog/using-testing-library-jest-dom-with-vitest
- 9: https://testing-library.com/docs/user-event/intro/
- 10: https://github.com/testing-library/react-testing-library
LGTM! Testing dependencies are correctly updated to the latest stable versions.
The versions listed inpackages/project-builder-web/package.json(lines 44-50 and 62) are in line with the latest release data as of February 2025:
- @testing-library/react: 16.2.0
- @testing-library/dom: 10.4.0
- @testing-library/jest-dom: 6.6.3
- @testing-library/user-event: 14.6.1 (aligns with the current recommendation for v14+)
These fixed versions enhance reproducibility and ensure the project's testing capabilities remain robust.
packages/project-builder-web/public/preview-app.json (6)
12-12: Consistent renaming of fields to use "Ref" suffix.The renaming of fields to include the "Ref" suffix (e.g.,
featureRef,modelFieldRef, etc.) improves clarity and better indicates references.Also applies to: 17-17, 25-25, 29-29, 37-37, 43-43, 51-51, 79-79, 95-95, 120-120, 126-126, 131-134, 137-140, 147-147
1256-1297: Well-structured file storage plugin configuration.The storage plugin configuration is well-organized with:
- Clear category definitions for different file types
- Appropriate file size limits
- Role-based upload permissions
- S3 adapter configuration with environment variables
31-34: New "Cover Photo" field added with proper file relation.The addition of the cover photo field is well-structured with:
- Clear label
- Proper model relation reference
- Appropriate file type
193-226: Enhanced auth configuration with improved role definitions.The auth configuration improvements include:
- Clear role definitions with helpful comments
- Built-in flag for system roles
- Proper references to user and role models
302-320: Comprehensive GraphQL configuration for models.The GraphQL configuration for each model is well-structured with:
- Clear field visibility control
- Role-based mutation permissions
- Proper relation exposure
Also applies to: 683-707, 819-835, 903-919, 1030-1037, 1101-1115, 1201-1208
1300-1300:Details
✅ Verification successful
Schema version increment from 4 to 9.
Major schema version increment suggests significant structural changes. Please ensure proper migration scripts are in place.
Let's check for migration scripts:
🏁 Script executed:
#!/bin/bash # Description: Check for migration scripts between schema versions 4 and 9 # Search for migration files fd -t f "migration" -x echo "Found migration file: {}" # Search for version-related code rg -A 5 "schemaVersion.*[49]"Length of output: 1891
Migration Scripts Are Present for Schema Version Change
The schema version bump from 4 to 9 implies significant changes. The verification indicates that multiple migration scripts (e.g.,
migration-009-rename-refs.ts, along with scripts numbered 005 to 008) exist within thepackages/project-builder-lib/src/migrations/directory. Additionally, corresponding unit tests are present and configurations in files liketests/simple/baseplate/project-definition.jsonreflect schema version 9. Please ensure that all individual migration steps (covering the gap from version 4 to 9) are thoroughly validated and that migration tests pass in your CI pipeline.
| // a bit of a hack to make sure we get the latest form state before continuing | ||
| // there's not an easy way of figuring out if the submission was successful or not | ||
| // so we wait for a React re-render and then check if the form is still dirty | ||
| await new Promise((resolve) => setTimeout(resolve, 1)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider a more robust approach for handling form submission state.
The current approach of waiting 1ms might be too short for React's batching behavior to complete, potentially leading to race conditions. The comment acknowledges this as a "hack", suggesting room for improvement.
Consider these alternatives:
- Use React Hook Form's submission status:
const { isSubmitting, isSubmitSuccessful } = useFormState({ control });- Or handle submission status explicitly:
const onContinue = async () => {
try {
await onSubmit();
return true;
} catch {
return false;
}
};
packages/project-builder-web/src/pages/data/models/_hooks/useModelForm.ts
Show resolved
Hide resolved
| export const projectsRouter = router({ | ||
| list: privateProcedure.query(async ({ ctx }) => | ||
| Promise.all( | ||
| ctx.services.map(async (service): Promise<ProjectInfo> => { | ||
| const { contents } = await service.readDefinition(); | ||
| const parsedContents = JSON.parse(contents) as unknown; | ||
|
|
||
| if ( | ||
| !parsedContents || | ||
| typeof parsedContents !== 'object' || | ||
| !('name' in parsedContents) || | ||
| typeof parsedContents.name !== 'string' | ||
| ) { | ||
| throw new Error( | ||
| `Invalid project definition for ${service.directory}`, | ||
| ); | ||
| } | ||
|
|
||
| return router({ | ||
| list: privateProcedure.query(async () => | ||
| Promise.all( | ||
| services.map( | ||
| async ( | ||
| service, | ||
| ): Promise<{ id: string; name: string; directory: string }> => { | ||
| const config = await service.readConfig(); | ||
| if (!config) { | ||
| throw new Error(`File config missing for ${service.directory}`); | ||
| } | ||
| const parsedContents = JSON.parse( | ||
| config.contents, | ||
| ) as ProjectDefinition; | ||
| return { | ||
| id: service.id, | ||
| name: parsedContents.name, | ||
| directory: service.directory, | ||
| }; | ||
| }, | ||
| ), | ||
| ), | ||
| return { | ||
| id: service.id, | ||
| name: parsedContents.name, | ||
| directory: service.directory, | ||
| }; | ||
| }), | ||
| ), | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle JSON parsing errors gracefully.
In list, the code currently trusts JSON.parse to work without issue. If the file contents are malformed, this will throw a runtime error. Consider wrapping the parsing in a try/catch block to provide a friendlier error or fallback behavior.
const { contents } = await service.readDefinition();
- const parsedContents = JSON.parse(contents) as unknown;
+ let parsedContents: unknown;
+ try {
+ parsedContents = JSON.parse(contents);
+ } catch (parseError) {
+ throw new Error(`Could not parse project definition for ${service.directory}: ${parseError}`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const projectsRouter = router({ | |
| list: privateProcedure.query(async ({ ctx }) => | |
| Promise.all( | |
| ctx.services.map(async (service): Promise<ProjectInfo> => { | |
| const { contents } = await service.readDefinition(); | |
| const parsedContents = JSON.parse(contents) as unknown; | |
| if ( | |
| !parsedContents || | |
| typeof parsedContents !== 'object' || | |
| !('name' in parsedContents) || | |
| typeof parsedContents.name !== 'string' | |
| ) { | |
| throw new Error( | |
| `Invalid project definition for ${service.directory}`, | |
| ); | |
| } | |
| return router({ | |
| list: privateProcedure.query(async () => | |
| Promise.all( | |
| services.map( | |
| async ( | |
| service, | |
| ): Promise<{ id: string; name: string; directory: string }> => { | |
| const config = await service.readConfig(); | |
| if (!config) { | |
| throw new Error(`File config missing for ${service.directory}`); | |
| } | |
| const parsedContents = JSON.parse( | |
| config.contents, | |
| ) as ProjectDefinition; | |
| return { | |
| id: service.id, | |
| name: parsedContents.name, | |
| directory: service.directory, | |
| }; | |
| }, | |
| ), | |
| ), | |
| return { | |
| id: service.id, | |
| name: parsedContents.name, | |
| directory: service.directory, | |
| }; | |
| }), | |
| ), | |
| ), | |
| export const projectsRouter = router({ | |
| list: privateProcedure.query(async ({ ctx }) => | |
| Promise.all( | |
| ctx.services.map(async (service): Promise<ProjectInfo> => { | |
| const { contents } = await service.readDefinition(); | |
| let parsedContents: unknown; | |
| try { | |
| parsedContents = JSON.parse(contents); | |
| } catch (parseError) { | |
| throw new Error(`Could not parse project definition for ${service.directory}: ${parseError}`); | |
| } | |
| if ( | |
| !parsedContents || | |
| typeof parsedContents !== 'object' || | |
| !('name' in parsedContents) || | |
| typeof parsedContents.name !== 'string' | |
| ) { | |
| throw new Error( | |
| `Invalid project definition for ${service.directory}`, | |
| ); | |
| } | |
| return { | |
| id: service.id, | |
| name: parsedContents.name, | |
| directory: service.directory, | |
| }; | |
| }), | |
| ), | |
| ), | |
| }); |
| onConsoleEmitted: websocketProcedure | ||
| .input(z.object({ id: z.string() })) | ||
| .subscription(({ input: { id }, ctx }) => | ||
| observable<CommandConsoleEmittedPayload>((emit) => { | ||
| const unsubscribe = ctx | ||
| .getApi(id) | ||
| .on('command-console-emitted', (payload) => { | ||
| emit.next(payload); | ||
| }); | ||
| return () => { | ||
| unsubscribe(); | ||
| }; | ||
| }), | ||
|
|
||
| onConsoleEmitted: websocketProcedure | ||
| .input(z.object({ id: z.string() })) | ||
| .subscription(({ input: { id } }) => | ||
| observable<CommandConsoleEmittedPayload>((emit) => { | ||
| const unsubscribe = getApi(id).on( | ||
| 'command-console-emitted', | ||
| (payload) => { | ||
| emit.next(payload); | ||
| }, | ||
| ); | ||
| return () => { | ||
| unsubscribe(); | ||
| }; | ||
| }), | ||
| ), | ||
| }); | ||
| } | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for getApi.
The subscription should handle potential errors from getApi to prevent unhandled exceptions.
.subscription(({ input: { id }, ctx }) =>
observable<CommandConsoleEmittedPayload>((emit) => {
+ try {
const unsubscribe = ctx
.getApi(id)
.on('command-console-emitted', (payload) => {
emit.next(payload);
});
return () => {
unsubscribe();
};
+ } catch (error) {
+ ctx.logger.error('Failed to setup console subscription:', error);
+ emit.error(new Error('Failed to setup console subscription'));
+ return () => {};
+ }
}),
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onConsoleEmitted: websocketProcedure | |
| .input(z.object({ id: z.string() })) | |
| .subscription(({ input: { id }, ctx }) => | |
| observable<CommandConsoleEmittedPayload>((emit) => { | |
| const unsubscribe = ctx | |
| .getApi(id) | |
| .on('command-console-emitted', (payload) => { | |
| emit.next(payload); | |
| }); | |
| return () => { | |
| unsubscribe(); | |
| }; | |
| }), | |
| onConsoleEmitted: websocketProcedure | |
| .input(z.object({ id: z.string() })) | |
| .subscription(({ input: { id } }) => | |
| observable<CommandConsoleEmittedPayload>((emit) => { | |
| const unsubscribe = getApi(id).on( | |
| 'command-console-emitted', | |
| (payload) => { | |
| emit.next(payload); | |
| }, | |
| ); | |
| return () => { | |
| unsubscribe(); | |
| }; | |
| }), | |
| ), | |
| }); | |
| } | |
| ), | |
| onConsoleEmitted: websocketProcedure | |
| .input(z.object({ id: z.string() })) | |
| .subscription(({ input: { id }, ctx }) => | |
| observable<CommandConsoleEmittedPayload>((emit) => { | |
| try { | |
| const unsubscribe = ctx | |
| .getApi(id) | |
| .on('command-console-emitted', (payload) => { | |
| emit.next(payload); | |
| }); | |
| return () => { | |
| unsubscribe(); | |
| }; | |
| } catch (error) { | |
| ctx.logger.error('Failed to setup console subscription:', error); | |
| emit.error(new Error('Failed to setup console subscription')); | |
| return () => {}; | |
| } | |
| }), | |
| ), |
| const api = ctx.getApi(id); | ||
|
|
||
| api.buildProject().catch((error: unknown) => { | ||
| logger.error(error); | ||
| }); | ||
| api.buildProject().catch((error: unknown) => { | ||
| ctx.logger.error(error); | ||
| }); | ||
|
|
||
| return { success: true, writeResult }; | ||
| return { success: true }; | ||
| }), | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and response structure.
The current implementation has room for improvement in error handling and response structure.
.mutation(({ input: { id }, ctx }) => {
const api = ctx.getApi(id);
- api.buildProject().catch((error: unknown) => {
- ctx.logger.error(error);
- });
+ return api.buildProject()
+ .then(() => ({ success: true, status: 'completed' }))
+ .catch((error: unknown) => {
+ ctx.logger.error('Build project failed:', error);
+ throw new Error('Failed to build project');
+ });
- return { success: true };
}),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .mutation(({ input: { id }, ctx }) => { | |
| const api = ctx.getApi(id); | |
| api.buildProject().catch((error: unknown) => { | |
| logger.error(error); | |
| }); | |
| api.buildProject().catch((error: unknown) => { | |
| ctx.logger.error(error); | |
| }); | |
| return { success: true, writeResult }; | |
| return { success: true }; | |
| }), | |
| .mutation(({ input: { id }, ctx }) => { | |
| const api = ctx.getApi(id); | |
| return api.buildProject() | |
| .then(() => ({ success: true, status: 'completed' })) | |
| .catch((error: unknown) => { | |
| ctx.logger.error('Build project failed:', error); | |
| throw new Error('Failed to build project'); | |
| }); | |
| }), |
| const { migratedDefinition, appliedMigrations } = runSchemaMigrations( | ||
| projectJson as ProjectDefinition, | ||
| ); | ||
| if (appliedMigrations.length > 0) { | ||
| await writeFile( | ||
| projectJsonPath, | ||
| prettyStableStringify(migratedProjectJson), | ||
| ); | ||
| await writeFile(projectJsonPath, prettyStableStringify(migratedDefinition)); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify type assertion safety.
The type assertion as ProjectDefinition could be unsafe. Consider adding runtime validation before the assertion.
🏁 Script executed:
#!/bin/bash
# Search for other instances of this type assertion to ensure consistent validation
rg -A 2 "as ProjectDefinition" Length of output: 555
Action Required: Integrate Runtime Validation for ProjectDefinition Instances
The type assertions in use (e.g., converting projectJson as ProjectDefinition in packages/project-builder-server/src/runner/index.ts and casting the result of migration.migrate(migratedDefinition) in packages/project-builder-lib/src/migrations/index.ts) currently rely solely on compile-time checks and lack any runtime verification. This approach may lead to unsafe assumptions about the structure of the data.
- Runner Module (packages/project-builder-server/src/runner/index.ts):
• Validate thatprojectJsonconforms to theProjectDefinitionschema before asserting its type. - Migrations Module (packages/project-builder-lib/src/migrations/index.ts):
• After performing migration viamigration.migrate(migratedDefinition), ensure the result meets the expectations ofProjectDefinitionwith runtime validation.
Recommendation: Consider using a runtime schema validation library (such as zod or io-ts) to perform these checks and safeguard against invalid data shapes.
| const orderedEntities = toposort.array( | ||
| entitiesWithNameResolver.map((entity) => entity.id), | ||
| entitiesWithNameResolver.flatMap((entity) => { | ||
| const entityIds = entity.nameResolver.idsToResolve ?? {}; | ||
| return Object.values(entityIds) | ||
| .flat() | ||
| .filter((id) => id in entitiesById) | ||
| .map((id) => [id, entity.id] as [string, string]); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for cyclic dependencies.
The toposort operation could fail with cyclic dependencies. Consider wrapping it in a try-catch block to provide a more user-friendly error message.
- const orderedEntities = toposort.array(
+ let orderedEntities: string[];
+ try {
+ orderedEntities = toposort.array(
entitiesWithNameResolver.map((entity) => entity.id),
entitiesWithNameResolver.flatMap((entity) => {
const entityIds = entity.nameResolver.idsToResolve ?? {};
return Object.values(entityIds)
.flat()
.filter((id) => id in entitiesById)
.map((id) => [id, entity.id] as [string, string]);
}),
);
+ } catch (error) {
+ throw new Error('Cyclic dependency detected in entity references. Please check your entity relationships.');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const orderedEntities = toposort.array( | |
| entitiesWithNameResolver.map((entity) => entity.id), | |
| entitiesWithNameResolver.flatMap((entity) => { | |
| const entityIds = entity.nameResolver.idsToResolve ?? {}; | |
| return Object.values(entityIds) | |
| .flat() | |
| .filter((id) => id in entitiesById) | |
| .map((id) => [id, entity.id] as [string, string]); | |
| }), | |
| ); | |
| let orderedEntities: string[]; | |
| try { | |
| orderedEntities = toposort.array( | |
| entitiesWithNameResolver.map((entity) => entity.id), | |
| entitiesWithNameResolver.flatMap((entity) => { | |
| const entityIds = entity.nameResolver.idsToResolve ?? {}; | |
| return Object.values(entityIds) | |
| .flat() | |
| .filter((id) => id in entitiesById) | |
| .map((id) => [id, entity.id] as [string, string]); | |
| }), | |
| ); | |
| } catch (error) { | |
| throw new Error('Cyclic dependency detected in entity references. Please check your entity relationships.'); | |
| } |
| const { definition, definitionContainer } = useProjectDefinition(); | ||
| const model = modelRef | ||
| ? ModelUtils.byIdOrThrow(definition, modelRef) | ||
| : undefined; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify error handling for ModelUtils.byIdOrThrow.
The switch from getModelById to ModelUtils.byIdOrThrow introduces stricter error handling. Ensure that error boundaries or try-catch blocks are in place to handle potential exceptions when the model is not found.
🏁 Script executed:
#!/bin/bash
# Search for error boundary components or try-catch blocks in parent components
rg -A 5 "ErrorBoundary|try\s*{" packages/project-builder-web/src/pages/apps/edit/admin/crud/Length of output: 200
Next Steps: Verify Error Handling for ModelUtils.byIdOrThrow Usage
The switch to using ModelUtils.byIdOrThrow means that an exception will be raised when the model is not found. Our initial attempt to locate error boundaries or try-catch blocks in the parent components (under packages/project-builder-web/src/pages/apps/edit/admin/crud/) encountered a regex error. To move forward, please run the following revised shell script to check for any existing error handling constructs:
Once you have the output:
- If there are try-catch blocks or an
ErrorBoundarywrapping the code that callsModelUtils.byIdOrThrow, please confirm that they are adequate to catch the potential exceptions. - If not, consider adding localized error handling (e.g., wrapping the call in a try-catch) or ensuring that a higher-level ErrorBoundary component is in place to handle these exceptions.
Please verify the results and update the error handling accordingly.
🏁 Script executed:
#!/bin/bash
echo "Searching for try-catch blocks in admin/crud directory:"
rg -A 5 'try\s*\{' packages/project-builder-web/src/pages/apps/edit/admin/crud/
echo "Searching for ErrorBoundary usage in admin/crud directory:"
rg -A 5 'ErrorBoundary' packages/project-builder-web/src/pages/apps/edit/admin/crud/Length of output: 414
Enhance Error Handling for ModelUtils.byIdOrThrow
Our investigation shows that no try-catch blocks or ErrorBoundary components are present in the packages/project-builder-web/src/pages/apps/edit/admin/crud/ directory to capture exceptions thrown by ModelUtils.byIdOrThrow. This leaves the current implementation exposed to unhandled errors if the model is not found.
Suggestions:
- Wrap the call in a try-catch block: At the point where
ModelUtils.byIdOrThrowis invoked (lines 92-95 inCrudTableColumnsForm.tsx), add error handling to catch potential exceptions. - Or Use an ErrorBoundary: Consider implementing or ensuring an existing ErrorBoundary component wraps this part of the UI to gracefully handle errors and prevent the entire app from crashing.
Please update the error handling to address this potential issue.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
.changeset/strange-toes-attend.md (1)
5-6: Consider enhancing the changeset description.While the description captures the core change, it would be helpful to include:
- Motivation for the change
- Impact on existing code
- Any migration steps if needed
Apply this diff to enhance the description:
-Refactor nameRefs to use resolveName for resolving name of entities +Refactor nameRefs to use resolveName for resolving name of entities. + +This change: +- Improves code clarity by centralizing name resolution logic +- Maintains backward compatibility with existing implementations +- Aligns with the new project definition sync/save flowpackages/project-builder-web/src/services/api/errors.ts (1)
13-28: Consider adding type narrowing for better type safety.The function is well-structured with proper documentation and error handling. However, the optional chaining on
err.data?.codecould be improved with type narrowing.Consider this improvement:
export function createProjectNotFoundHandler( projectId: string, ): (err: unknown) => never { return (err: unknown) => { - if (isTRPCClientError(err) && err.data?.code === 'NOT_FOUND') { + if (isTRPCClientError(err) && 'data' in err && err.data && err.data.code === 'NOT_FOUND') { throw new ProjectNotFoundError(projectId); } throw err; }; }packages/project-builder-web/src/pages/data/models/_hooks/useModelForm.ts (2)
102-127: Enhance error messages for better user guidance.While the validation logic is thorough, the error messages could be more descriptive and provide guidance on how to fix the issues.
Apply this diff to improve error messages:
- toast.error('Model must have at least one field.'); + toast.error('Model validation failed: At least one field is required. Please add a field to continue.'); - toast.error('Model must have at least one primary key field.'); + toast.error('Model validation failed: A primary key field is required for database operations. Please select at least one field as primary key.'); - message: `Model with name ${updatedModel.name} already exists.`, + message: `A model named "${updatedModel.name}" already exists. Please choose a unique name.`,
129-164: Refactor service method validation to reduce duplication.The validation logic for service methods could be improved by:
- Extracting duplicated validation logic into a helper function.
- Enhancing error messages with more context.
Apply this diff to improve the code:
+ // Helper function to validate service method + const validateServiceMethod = ( + method: { enabled?: boolean; fields?: string[]; transformerNames?: string[] }, + methodName: string, + ): boolean => { + if (method?.enabled && !method.fields?.length && !method.transformerNames?.length) { + toast.error( + `${methodName} method validation failed: At least one field or transformer is required.` + + ` Please configure the ${methodName.toLowerCase()} method or disable it.` + ); + return false; + } + return true; + }; + // clear out any service methods that are disabled const { service } = updatedModel; if (service) { - if (service.create?.enabled) { - if ( - !service.create.fields?.length && - !service.create.transformerNames?.length - ) { - toast.error( - 'Create method must have at least one field or transformer.', - ); - return; - } - } else { + if (!validateServiceMethod(service.create, 'Create')) { + return; + } + if (!service.create?.enabled) { service.create = undefined; } - if (service.update?.enabled) { - if ( - !service.update.fields?.length && - !service.update.transformerNames?.length - ) { - toast.error( - 'Update method must have at least one field or transformer.', - ); - return; - } - } else { + if (!validateServiceMethod(service.update, 'Update')) { + return; + } + if (!service.update?.enabled) { service.update = undefined; }packages/tools/eslint-configs/tailwind.js (1)
23-24: Consider documenting the VSCode error.The comment about VSCode throwing an error is helpful, but consider documenting the specific error and its workaround in your project's documentation for future reference.
.changeset/six-candles-accept.md (1)
7-8: Enhance the changeset description.While the current description captures the main purpose, it would be helpful to include:
- The motivation behind the changes
- The specific improvements made to the sync/save flow
- The impact on consumers of these packages
Consider expanding the description like this:
-Rewrite saving/syncing logic to clean up flows for parsing and saving project definition +Rewrite saving/syncing logic to improve project definition handling: +- Implement asynchronous saving capabilities +- Refactor nameRefs to name resolvers for Zod reference payloads +- Create clearer flow for parsing and saving project definitions + +This change improves code maintainability and provides a more intuitive API for consumers.packages/project-builder-lib/src/tools/model-merger/model-merger.ts (4)
77-107: Consider optimizing array operations for better performance.The current implementation has O(n²) time complexity due to nested array operations (find, some). For large arrays, this could impact performance.
Consider using a Map for O(1) lookups:
function diffByKey<T>( current: T[], desired: T[], getKey: (item: T) => string, options: ModelDiffOptions = {}, ): DiffOperation<T>[] { const ops: DiffOperation<T>[] = []; + const currentMap = new Map(current.map(item => [getKey(item), item])); + const desiredKeys = new Set(desired.map(getKey)); for (const desiredItem of desired) { const key = getKey(desiredItem); - const currentItem = current.find((item) => getKey(item) === key); + const currentItem = currentMap.get(key); if (!currentItem) { ops.push({ type: 'add', key, item: desiredItem }); continue; } if (!isMatch(currentItem, desiredItem)) { ops.push({ type: 'update', key, item: desiredItem }); } } if (options.enableRemove) { - for (const currentItem of current) { - const key = getKey(currentItem); - if (!desired.some((item) => getKey(item) === key)) { + for (const [key, currentItem] of currentMap) { + if (!desiredKeys.has(key)) { ops.push({ type: 'remove', key, item: currentItem }); } } } return ops; }
118-151: Consider enhancing error handling and immutability.While the function works correctly, there are two areas for improvement:
- The function mutates the array in-place during splice operations
- Error handling is only implemented for the update operation
Consider this more robust implementation:
function applyPatchByKey<TConfig, TInput>( current: TConfig[], patch: DiffOperation<TInput>[], getKey: (item: TConfig) => string, assignId: (item: TInput, previousItem?: TConfig) => TConfig, ): TConfig[] { - const items = [...current]; + let items = [...current]; for (const { type, key, item } of patch) { const index = items.findIndex((i) => getKey(i) === key); switch (type) { case 'add': { - items.push(assignId(item)); + items = [...items, assignId(item)]; break; } case 'update': { if (index === -1) { throw new Error( `Cannot apply patch. Item with key "${key}" not found.`, ); } // Preserve existing id if present. - items[index] = assignId(item, items[index]); + items = [ + ...items.slice(0, index), + assignId(item, items[index]), + ...items.slice(index + 1), + ]; break; } case 'remove': { if (index === -1) { + throw new Error( + `Cannot remove item. Item with key "${key}" not found.`, + ); + } - if (index !== -1) { - items.splice(index, 1); - } + items = [...items.slice(0, index), ...items.slice(index + 1)]; break; } } } return items; }
159-164: Consider making the unique constraint key more robust.The current implementation might have issues if field references contain the pipe character used as a separator.
Consider using a more robust separator or encoding:
function getUniqueConstraintKey( constraint: ModelUniqueConstraintDefinitionInput, ): string { const fields = constraint.fields.map((f) => f.fieldRef).sort(); - return fields.join('|'); + // Use a character that's unlikely to appear in field names + return fields.join('\u0000'); // Alternative: Use JSON.stringify for complete safety + // return JSON.stringify(fields); }
249-323: Optimize ID resolution for better performance.The current implementation performs repeated array searches for ID resolution. This could be optimized by caching the field lookups.
Consider this optimization:
export function applyModelPatchInPlace( current: ModelConfig['model'], patch: ModelDiffOutput, definitionContainer: ProjectDefinitionContainer, ): void { + // Cache field lookups + const fieldMap = new Map(current.fields.map(f => [f.name, f])); + // Resolve relation references. const resolveLocalId = (name: string): string => { - const field = current.fields.find((f) => f.name === name); + const field = fieldMap.get(name); return field ? field.id : name; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
.changeset/six-candles-accept.md(1 hunks).changeset/strange-toes-attend.md(1 hunks)packages/project-builder-lib/src/compiler/app-compiler-spec.ts(1 hunks)packages/project-builder-lib/src/tools/model-merger/model-merger.ts(1 hunks)packages/project-builder-lib/src/utils/merge.ts(0 hunks)packages/project-builder-web/package.json(2 hunks)packages/project-builder-web/src/pages/data/models/_hooks/useModelForm.ts(4 hunks)packages/project-builder-web/src/services/api/errors.ts(1 hunks)packages/project-builder-web/src/services/config.ts(1 hunks)packages/tools/eslint-configs/tailwind.js(1 hunks)
💤 Files with no reviewable changes (1)
- packages/project-builder-lib/src/utils/merge.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/project-builder-web/src/services/config.ts
- packages/project-builder-web/package.json
- packages/project-builder-lib/src/compiler/app-compiler-spec.ts
🔇 Additional comments (7)
.changeset/strange-toes-attend.md (1)
1-3: LGTM! Patch version bump is appropriate.The patch version bump aligns with semantic versioning guidelines for backward-compatible refactors.
packages/project-builder-web/src/services/api/errors.ts (2)
1-1: LGTM!The import statement is correctly defined and used within the file.
3-11: LGTM! Well-structured error class.The class follows best practices by:
- Extending the Error base class
- Including a descriptive error message
- Setting the name property for proper error identification
- Having proper JSDoc documentation
packages/project-builder-web/src/pages/data/models/_hooks/useModelForm.ts (2)
23-23: LGTM! Improved error handling with custom error type.Good use of a custom error type for better error handling and user feedback.
166-196: LGTM! Well-structured save and navigation logic.The implementation provides:
- Clear success messages based on the operation type (create/update)
- Proper form reset and navigation after successful submission
- Appropriate callback handling
.changeset/six-candles-accept.md (1)
2-4:Details
✅ Verification successful
Verify if patch version is appropriate for these changes.
The PR involves a significant rewrite of sync/save flows. While the changes aim to improve clarity, please verify that they maintain backward compatibility and don't introduce breaking changes that would require a minor or major version bump.
Run this script to check for potential breaking changes:
🏁 Script executed:
#!/bin/bash # Description: Check for potential breaking changes in the affected packages # Look for removed or renamed exports rg -i "export.*\b(class|interface|type|function|const)\b.*\b(ProjectDefinition|nameRefs|ParsedProjectDefinition)\b" # Look for signature changes in public APIs ast-grep --pattern $'export (class|interface|type|function|const) $_ { $$$ }'Length of output: 874
Patch Bump Confirmed – Consider Enhancing Description Details
- The verification scripts show that all exported APIs (e.g., the functions and types linked with ProjectDefinition) remain unchanged, indicating no breaking changes.
- Since no removed or signature-altered exports were found, the patch version bumps for the affected packages are appropriate.
- It could be helpful to expand the change description in this changeset to clarify the impact of the sync/save flow rewrite.
packages/project-builder-lib/src/tools/model-merger/model-merger.ts (1)
1-67: LGTM! Well-structured type definitions.The type definitions are clear, comprehensive, and follow TypeScript best practices. The use of the Omit utility type effectively prevents ID field conflicts in input types.
| 'tailwindcss/no-custom-classname': [ | ||
| 'error', | ||
| { | ||
| // for some reason, VSCode throws an error for this rule so adding the rules manually | ||
| whitelist: [ | ||
| 'text-muted-foreground', | ||
| 'text-destructive', | ||
| 'text-style-muted', | ||
| 'bg-background', | ||
| 'bg-muted', | ||
| 'border-border', | ||
| 'text-style-prose', | ||
| ], | ||
| }, | ||
| ], |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM! Verify whitelist against Tailwind config.
The whitelist configuration looks good and follows consistent naming patterns. However, ensure these classes are properly defined in your Tailwind configuration file.
Let's verify the class definitions in your Tailwind configuration:
🏁 Script executed:
#!/bin/bash
# Search for the Tailwind configuration file and check for these class definitions
fd -e js -e ts "tailwind.config" -x cat {} | grep -E "text-muted-foreground|text-destructive|text-style-muted|bg-background|bg-muted|border-border|text-style-prose"Length of output: 228
Action Required: Verify and update whitelist entries in ESLint config.
The ESLint rule for tailwindcss/no-custom-classname is correctly structured, and the comment on the VSCode issue is useful. However, a search of the Tailwind configuration revealed that only the classes corresponding to text-style-muted and text-style-prose are defined—while no definitions were found for text-muted-foreground, text-destructive, bg-background, bg-muted, or border-border. Please verify whether these additional classes are intended to be recognized (perhaps defined elsewhere or pending definition) or if they should be removed/updated to align with the Tailwind configuration.
| export function diffModel( | ||
| current: ModelConfig['model'], | ||
| desired: ModelDefinitionInput, | ||
| definitionContainer: ProjectDefinitionContainer, | ||
| options?: ModelDiffOptions, | ||
| ): ModelDiffOutput | undefined { | ||
| // Resolve relation references. | ||
| const resolvedRelations = | ||
| current.relations?.map((relation) => ({ | ||
| ...relation, | ||
| references: relation.references.map((reference) => ({ | ||
| ...reference, | ||
| localRef: definitionContainer.nameFromId(reference.localRef), | ||
| foreignRef: definitionContainer.nameFromId(reference.foreignRef), | ||
| })), | ||
| })) ?? []; | ||
|
|
||
| const fieldDiffs = diffByKey( | ||
| current.fields, | ||
| desired.fields, | ||
| (f) => f.name, | ||
| options, | ||
| ); | ||
| const relationDiffs = diffByKey( | ||
| resolvedRelations, | ||
| desired.relations ?? [], | ||
| (r) => r.name, | ||
| options, | ||
| ); | ||
| const uniqueConstraintDiffs = diffByKey( | ||
| (current.uniqueConstraints ?? []).map((constraint) => ({ | ||
| ...constraint, | ||
| fields: constraint.fields.map((f) => ({ | ||
| ...f, | ||
| fieldRef: definitionContainer.nameFromId(f.fieldRef), | ||
| })), | ||
| })), | ||
| desired.uniqueConstraints ?? [], | ||
| getUniqueConstraintKey, | ||
| options, | ||
| ); | ||
|
|
||
| const pkDiff = isEqual( | ||
| current.primaryKeyFieldRefs.map((ref) => | ||
| definitionContainer.nameFromId(ref), | ||
| ), | ||
| desired.primaryKeyFieldRefs, | ||
| ) | ||
| ? undefined | ||
| : desired.primaryKeyFieldRefs; | ||
|
|
||
| if ( | ||
| fieldDiffs.length > 0 || | ||
| relationDiffs.length > 0 || | ||
| uniqueConstraintDiffs.length > 0 || | ||
| pkDiff | ||
| ) { | ||
| return { | ||
| fields: fieldDiffs, | ||
| relations: relationDiffs, | ||
| uniqueConstraints: uniqueConstraintDiffs, | ||
| primaryKeyFieldRefs: pkDiff, | ||
| }; | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add error handling for name resolution failures.
The name resolution using definitionContainer.nameFromId could fail silently if an ID doesn't exist.
Let's verify the name resolution behavior:
Consider adding error handling:
export function diffModel(
current: ModelConfig['model'],
desired: ModelDefinitionInput,
definitionContainer: ProjectDefinitionContainer,
options?: ModelDiffOptions,
): ModelDiffOutput | undefined {
+ const safeNameFromId = (id: string): string => {
+ const name = definitionContainer.nameFromId(id);
+ if (!name) {
+ throw new Error(`Failed to resolve name for ID: ${id}`);
+ }
+ return name;
+ };
+
// Resolve relation references.
const resolvedRelations =
current.relations?.map((relation) => ({
...relation,
references: relation.references.map((reference) => ({
...reference,
- localRef: definitionContainer.nameFromId(reference.localRef),
- foreignRef: definitionContainer.nameFromId(reference.foreignRef),
+ localRef: safeNameFromId(reference.localRef),
+ foreignRef: safeNameFromId(reference.foreignRef),
})),
})) ?? [];🏁 Script executed:
#!/bin/bash
# Search for error handling in nameFromId implementations
rg -A 5 "nameFromId.*\{" Length of output: 2762
Enforce Strict Error Handling for Name Resolution in diffModel
The current implementation directly calls definitionContainer.nameFromId when processing relation references. Although nameFromId already throws an error if it can’t resolve a given non-empty ID, it returns undefined if passed an undefined value. This silent failure for undefined IDs may lead to unintended behavior in the diff results. I recommend introducing a safe wrapper—such as a safeNameFromId helper—that ensures every call to resolve an ID returns a valid name (throwing an error otherwise). For example:
export function diffModel(
current: ModelConfig['model'],
desired: ModelDefinitionInput,
definitionContainer: ProjectDefinitionContainer,
options?: ModelDiffOptions,
): ModelDiffOutput | undefined {
+ const safeNameFromId = (id: string | undefined): string => {
+ const name = definitionContainer.nameFromId(id);
+ if (!name) {
+ throw new Error(`Failed to resolve a valid name for ID: ${id}`);
+ }
+ return name;
+ };
+
// Resolve relation references.
const resolvedRelations =
current.relations?.map((relation) => ({
...relation,
references: relation.references.map((reference) => ({
...reference,
- localRef: definitionContainer.nameFromId(reference.localRef),
- foreignRef: definitionContainer.nameFromId(reference.foreignRef),
+ localRef: safeNameFromId(reference.localRef),
+ foreignRef: safeNameFromId(reference.foreignRef),
})),
})) ?? [];This change will ensure that any unresolved or missing IDs (even those that might be undefined) are caught early and handled explicitly rather than failing silently.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function diffModel( | |
| current: ModelConfig['model'], | |
| desired: ModelDefinitionInput, | |
| definitionContainer: ProjectDefinitionContainer, | |
| options?: ModelDiffOptions, | |
| ): ModelDiffOutput | undefined { | |
| // Resolve relation references. | |
| const resolvedRelations = | |
| current.relations?.map((relation) => ({ | |
| ...relation, | |
| references: relation.references.map((reference) => ({ | |
| ...reference, | |
| localRef: definitionContainer.nameFromId(reference.localRef), | |
| foreignRef: definitionContainer.nameFromId(reference.foreignRef), | |
| })), | |
| })) ?? []; | |
| const fieldDiffs = diffByKey( | |
| current.fields, | |
| desired.fields, | |
| (f) => f.name, | |
| options, | |
| ); | |
| const relationDiffs = diffByKey( | |
| resolvedRelations, | |
| desired.relations ?? [], | |
| (r) => r.name, | |
| options, | |
| ); | |
| const uniqueConstraintDiffs = diffByKey( | |
| (current.uniqueConstraints ?? []).map((constraint) => ({ | |
| ...constraint, | |
| fields: constraint.fields.map((f) => ({ | |
| ...f, | |
| fieldRef: definitionContainer.nameFromId(f.fieldRef), | |
| })), | |
| })), | |
| desired.uniqueConstraints ?? [], | |
| getUniqueConstraintKey, | |
| options, | |
| ); | |
| const pkDiff = isEqual( | |
| current.primaryKeyFieldRefs.map((ref) => | |
| definitionContainer.nameFromId(ref), | |
| ), | |
| desired.primaryKeyFieldRefs, | |
| ) | |
| ? undefined | |
| : desired.primaryKeyFieldRefs; | |
| if ( | |
| fieldDiffs.length > 0 || | |
| relationDiffs.length > 0 || | |
| uniqueConstraintDiffs.length > 0 || | |
| pkDiff | |
| ) { | |
| return { | |
| fields: fieldDiffs, | |
| relations: relationDiffs, | |
| uniqueConstraints: uniqueConstraintDiffs, | |
| primaryKeyFieldRefs: pkDiff, | |
| }; | |
| } | |
| return undefined; | |
| } | |
| export function diffModel( | |
| current: ModelConfig['model'], | |
| desired: ModelDefinitionInput, | |
| definitionContainer: ProjectDefinitionContainer, | |
| options?: ModelDiffOptions, | |
| ): ModelDiffOutput | undefined { | |
| const safeNameFromId = (id: string | undefined): string => { | |
| const name = definitionContainer.nameFromId(id); | |
| if (!name) { | |
| throw new Error(`Failed to resolve a valid name for ID: ${id}`); | |
| } | |
| return name; | |
| }; | |
| // Resolve relation references. | |
| const resolvedRelations = | |
| current.relations?.map((relation) => ({ | |
| ...relation, | |
| references: relation.references.map((reference) => ({ | |
| ...reference, | |
| localRef: safeNameFromId(reference.localRef), | |
| foreignRef: safeNameFromId(reference.foreignRef), | |
| })), | |
| })) ?? []; | |
| const fieldDiffs = diffByKey( | |
| current.fields, | |
| desired.fields, | |
| (f) => f.name, | |
| options, | |
| ); | |
| const relationDiffs = diffByKey( | |
| resolvedRelations, | |
| desired.relations ?? [], | |
| (r) => r.name, | |
| options, | |
| ); | |
| const uniqueConstraintDiffs = diffByKey( | |
| (current.uniqueConstraints ?? []).map((constraint) => ({ | |
| ...constraint, | |
| fields: constraint.fields.map((f) => ({ | |
| ...f, | |
| fieldRef: definitionContainer.nameFromId(f.fieldRef), | |
| })), | |
| })), | |
| desired.uniqueConstraints ?? [], | |
| getUniqueConstraintKey, | |
| options, | |
| ); | |
| const pkDiff = isEqual( | |
| current.primaryKeyFieldRefs.map((ref) => | |
| definitionContainer.nameFromId(ref), | |
| ), | |
| desired.primaryKeyFieldRefs, | |
| ) | |
| ? undefined | |
| : desired.primaryKeyFieldRefs; | |
| if ( | |
| fieldDiffs.length > 0 || | |
| relationDiffs.length > 0 || | |
| uniqueConstraintDiffs.length > 0 || | |
| pkDiff | |
| ) { | |
| return { | |
| fields: fieldDiffs, | |
| relations: relationDiffs, | |
| uniqueConstraints: uniqueConstraintDiffs, | |
| primaryKeyFieldRefs: pkDiff, | |
| }; | |
| } | |
| return undefined; | |
| } |
kingston
deleted the
kingston/eng-469-when-syncing-and-saving-recently-modified-error-shows-up
branch
Summary by CodeRabbit
New Features
Improvements