Conversation
|
@claude review |
|
Claude finished @artemdemo's task in 1m 9s —— View job Code ReviewTasks:
SummaryThis PR simplifies the function schema by:
Issues Found🚨 Critical: Breaking ChangeThe PR introduces a breaking bug by renaming // src/core/resources/function/deploy.ts:15
fn.files.map(async (filePath) => {
// This will fail at runtime!
})This will cause a runtime error when attempting to deploy functions because Fix required: Update Positive Changes✅ Improved Schema SimplificationRemoving the redundant validation in Before (lines 45-57): const functionData = { ...config, entryPath, files };
const result = FunctionSchema.safeParse(functionData);
if (!result.success) {
throw new SchemaValidationError("Invalid function", result.error, configPath);
}
return result.data;After (lines 50-51): const functionData: BackendFunction = { ...config, entryPath, filePaths };
return functionData;This is valid because:
✅ Better Naming
RecommendationsTest CoverageThe existing test ( bun run build && bun run testThe test fixture at Consider TypeScript StrictnessThe bug would have been caught at compile time if TypeScript's type checking was stricter. The current code uses: async function loadFunctionCode(fn: BackendFunction): Promise<FunctionWithCode>TypeScript should error on
SummaryVerdict: ❌ Changes requested - Cannot merge due to breaking bug Action items:
The refactoring direction is good, but the incomplete property rename will break function deployments at runtime. |
🚀 Package Preview Available!Install this PR's preview build with npm: npm i @base44-preview/cli@0.0.28-pr.196.60a4444Prefer not to change any import paths? Install using npm alias so your code still imports npm i "base44@npm:@base44-preview/cli@0.0.28-pr.196.60a4444"Or add it to your {
"dependencies": {
"base44": "npm:@base44-preview/cli@0.0.28-pr.196.60a4444"
}
}
Preview published to npm registry — try new features instantly! |
| const functionData: BackendFunction = { ...config, entryPath, filePaths }; | ||
| return functionData; |
There was a problem hiding this comment.
It's enough to validate it based on paths - this is not user providing us with arbitrary data. It's our code
| export const BackendFunctionSchema = FunctionConfigSchema.extend({ | ||
| entryPath: z.string().min(1, "Entry path cannot be empty"), | ||
| files: z.array(z.string()).min(1, "Function must have at least one file"), | ||
| filePaths: z.array(z.string()).min(1, "Function must have at least one file"), |
There was a problem hiding this comment.
files used twice in schemas as "file path" and as actual "file content" I want to make names clear
| }); | ||
|
|
||
| export const FunctionSchema = FunctionConfigSchema.extend({ | ||
| export const BackendFunctionSchema = FunctionConfigSchema.extend({ |
There was a problem hiding this comment.
Let's call in BackendFunctionSchema since type then called BackendFunction
Note
Description
Refactors the function resource module to simplify schema validation and improve naming clarity. Removes redundant runtime Zod validation in
readFunction()since the config is already validated byFunctionConfigSchema. RenamesFunctionSchematoBackendFunctionSchemaand thefilesproperty tofilePathsfor better semantic accuracy.Related Issue
None
Type of Change
Changes Made
FunctionSchematoBackendFunctionSchemafor naming consistency with other resource schemasfilesproperty tofilePathsto accurately reflect that it stores file paths, not contentsreadFunction()- config is already validated byreadFunctionConfig()BackendFunctionobjectTesting
npm test)Checklist
Additional Notes
This refactoring removes defensive validation that was unnecessary since:
FunctionConfigSchemainreadFunctionConfig()entryPath,filePaths) are constructed internally and type-safeThe rename from
filestofilePathsmakes it clear that these are paths to files (strings), not file content objects.🤖 Generated by Claude | 2026-02-05 08:46 UTC