fix (run-time): Promise.all Parsing properly for Dynamic Arrays#166
fix (run-time): Promise.all Parsing properly for Dynamic Arrays#166
Conversation
Suggested PR title from PearlTitle: Body: Key Changes:
|
Deploying bubble-studio with
|
| Latest commit: |
2510aa2
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3e9935e2.bubble-studio.pages.dev |
| Branch Preview URL: | https://fix-promise-pushes.bubble-studio.pages.dev |
Deploying bubblelab-documentation with
|
| Latest commit: |
2510aa2
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ed93782d.bubblelab-documentation.pages.dev |
| Branch Preview URL: | https://fix-promise-pushes.bubblelab-documentation.pages.dev |
zhubzy
changed the title
zhubzy
changed the title
There was a problem hiding this comment.
Pull request overview
This PR addresses two main concerns: (1) enhancing the BubbleParser to handle Promise.all() with dynamically built arrays using .push() calls, and (2) adding a new lint rule to prevent methods from calling other methods within BubbleFlow classes.
Key Changes:
- Extended Promise.all() parsing to support array variables built with .push() calls, not just array literals
- Added
noMethodCallingMethodRulelint rule requiring that only thehandle()method calls other class methods - Changed default validation behavior in the API route to require lint errors by default
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
packages/bubble-runtime/src/extraction/BubbleParser.ts |
Added findArrayPushCalls() method to trace array.push() calls and updated detectPromiseAll() and buildParallelExecutionNode() to handle both ArrayExpression and Identifier types for Promise.all() arguments |
packages/bubble-runtime/src/validation/lint-rules.ts |
Added noMethodCallingMethodRule to enforce that only the handle() method can call other methods, preventing inter-method calls |
packages/bubble-runtime/src/validation/index.test.ts |
Added test case for the new lint rule validation |
apps/bubblelab-api/src/routes/bubble-flows.ts |
Removed the false parameter from validateAndExtract() call, changing behavior to require lint errors by default |
apps/bubble-studio/src/utils/workflowToSteps.test.ts |
Added integration test with example code using Promise.all() with array.push() pattern to verify workflow parsing |
| expect(response.ok).toBe(true); | ||
|
|
||
| const validationResult: ValidateBubbleFlowResponse = await response.json(); | ||
| console.log(JSON.stringify(validationResult.workflow, null, 2)); |
There was a problem hiding this comment.
[nitpick] The test uses console.log(JSON.stringify(validationResult.workflow, null, 2)) on line 40, which will output to console during test execution. This appears to be debugging code that should be removed.
| console.log(JSON.stringify(validationResult.workflow, null, 2)); |
| it('should fail validation when a method calls another method', async () => { | ||
| const code = ` | ||
| import { BubbleFlow, AIAgentBubble } from '@bubblelab/bubble-core'; | ||
|
|
||
| export class TestFlow extends BubbleFlow<'webhook/http'> { | ||
| private async helperMethod(): Promise<string> { | ||
| return 'test'; | ||
| } | ||
|
|
||
| private async gatherContext(): Promise<string> { | ||
| // This should fail - calling another method from a method | ||
| const result = await this.helperMethod(); | ||
| return result; | ||
| } | ||
|
|
||
| async handle(payload: any): Promise<any> { | ||
| const context = await this.gatherContext(); | ||
| return { context }; | ||
| } | ||
| } | ||
| `; | ||
| const result = await validateBubbleFlow(code); | ||
| expect(result.valid).toBe(false); | ||
| expect(result.errors).toBeDefined(); | ||
| expect( | ||
| result.errors?.some((error) => | ||
| error.includes('cannot be called from another method') | ||
| ) | ||
| ).toBe(true); | ||
| }); |
There was a problem hiding this comment.
The test case for the noMethodCallingMethodRule lacks comprehensive coverage. It only tests the positive case (a method calling another method should fail). Consider adding test cases for:
- The negative case: verify that methods NOT calling other methods pass validation
- Verify that the
handlemethod CAN call other methods without triggering errors - Test edge cases like property access on
thisthat aren't method calls (e.g.,this.someProperty)
| for (const key in node) { | ||
| const child = (node as any)[key]; | ||
| if (Array.isArray(child)) child.forEach(walk); | ||
| else if (child?.type) walk(child); | ||
| } |
There was a problem hiding this comment.
The tree walking implementation in findArrayPushCalls uses for (const key in node) which iterates over all enumerable properties, potentially including inherited properties from the prototype chain. This could lead to unexpected behavior or performance issues. Consider using a more robust AST traversal approach, such as checking for specific AST node properties (like body, expression, arguments, etc.) or using a library-provided traversal utility. Also consider using Object.hasOwnProperty() or Object.keys() to avoid prototype properties.
| /** | ||
| * Lint rule that prevents methods from calling other methods | ||
| * Methods should only be called from the handle method, not from other methods | ||
| */ | ||
| export const noMethodCallingMethodRule: LintRule = { | ||
| name: 'no-method-calling-method', | ||
| validate(context: LintRuleContext): LintError[] { | ||
| const errors: LintError[] = []; | ||
|
|
||
| if (!context.bubbleFlowClass) { | ||
| return errors; // No BubbleFlow class found, skip this rule | ||
| } | ||
|
|
||
| // Find all methods in the BubbleFlow class | ||
| const methods: ts.MethodDeclaration[] = []; | ||
| if (context.bubbleFlowClass.members) { | ||
| for (const member of context.bubbleFlowClass.members) { | ||
| if (ts.isMethodDeclaration(member)) { | ||
| // Skip the handle method - it's allowed to call other methods | ||
| const methodName = member.name; | ||
| if (ts.isIdentifier(methodName) && methodName.text === 'handle') { | ||
| continue; | ||
| } | ||
| methods.push(member); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // For each method, check if it calls other methods | ||
| for (const method of methods) { | ||
| if (!method.body || !ts.isBlock(method.body)) { | ||
| continue; | ||
| } | ||
|
|
||
| const methodCallErrors = findMethodCallsInNode( | ||
| method.body, | ||
| context.sourceFile | ||
| ); | ||
| errors.push(...methodCallErrors); | ||
| } | ||
|
|
||
| return errors; | ||
| }, | ||
| }; |
There was a problem hiding this comment.
[nitpick] The PR title is "fix: parsing for promise pushes" but this change adds an unrelated lint rule for preventing methods from calling other methods. This new lint rule should either be in a separate PR or the PR title should be updated to reflect all changes being made (e.g., "fix: parsing for promise pushes and add no-method-calling-method lint rule").
|
|
||
| // Create a new BubbleFlowValidationTool instance | ||
| const result = await validateAndExtract(code, bubbleFactory, false); | ||
| const result = await validateAndExtract(code, bubbleFactory); |
There was a problem hiding this comment.
The requireLintErrors parameter default value has changed from false to true (defaulting to true when no third parameter is provided). Ensure this behavior change is intentional and that any code relying on the previous default behavior is updated. This could cause previously passing validations to fail if they had lint errors.
| const result = await validateAndExtract(code, bubbleFactory); | |
| const result = await validateAndExtract(code, bubbleFactory, false); |
| this.findVariableIdByName(arrayVarName, pushLine, scopeManager) === | ||
| varId | ||
| ) { | ||
| pushedArgs.push(node.arguments[0] as TSESTree.Expression); |
There was a problem hiding this comment.
The findArrayPushCalls function only captures the first argument to .push() calls (line 3367). If a user calls array.push(item1, item2) with multiple arguments, only the first argument will be captured. While .push() can accept multiple arguments, this implementation will silently ignore subsequent arguments. Consider either documenting this limitation or handling multiple arguments.
| pushedArgs.push(node.arguments[0] as TSESTree.Expression); | |
| node.arguments.forEach(arg => { | |
| pushedArgs.push(arg as TSESTree.Expression); | |
| }); |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ameter for bubble flow validation
Summary
Enhanced Promise.all Parsing for Dynamic Arrays
Related Issues
Type of Change
Checklist
pnpm checkand all tests passScreenshots (if applicable)
Additional Context