Fix Promise.all visualization for .map() patterns#247
Fix Promise.all visualization for .map() patterns#247zhubzy merged 1 commit intobubblelabai:mainfrom
Conversation
📝 WalkthroughWalkthroughEnhanced BubbleParser to support Promise.all patterns using .map() callbacks in addition to existing .push() patterns. Includes expanded test coverage and a new fixture file demonstrating five distinct Promise.all pattern implementations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Suggested PR title from PearlTitle: Body: SummaryEnhances the BubbleParser to detect and parse Promise.all patterns that use ChangesParser Enhancements
New Helper Methods
Test CoverageAdded comprehensive test suite covering 5 Promise.all patterns:
Files Changed
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/bubble-runtime/src/extraction/BubbleParser.ts (3)
3883-3924: Consider documenting the map pattern detection limitations.The current implementation only detects
.map()patterns when the variable declaration directly matches the Promise.all array variable. This means intermediate variables won't be detected:// ✅ Detected const promises = items.map(x => this.process(x)); await Promise.all(promises); // ❌ Not detected const mapped = items.map(x => this.process(x)); const promises = mapped; await Promise.all(promises);Additionally, at lines 3915-3918, the same
callbackExpris duplicated for each source element without parameter substitution. While this appears intentional for visualization purposes, it means all parallel nodes will show identical expressions rather than reflecting the actual parameter values.Consider adding a comment explaining these limitations and the rationale for duplicating expressions without substitution.
3939-3957: Document callback expression extraction edge cases.The
extractCallbackExpressionmethod handles basic arrow and function expressions, but several edge cases aren't addressed:
- Async callbacks:
async (x) => await this.process(x)- Theasynckeyword isn't explicitly handled- Multiple return paths: Line 3954 only takes the first return statement, which may not represent all code paths
- Destructured parameters:
({x, y}) => this.process(x, y)While these may not be common in Promise.all patterns, consider adding a comment documenting these limitations.
3968-4018: Source array resolution is limited to literals and simple identifiers.The
getSourceArrayElementsmethod only resolves:
- Direct array literals:
[1, 2, 3]- Simple variable identifiers:
const arr = [1, 2, 3]; items.map(...)It doesn't handle:
- Function calls:
items.map(x => this.process(getArray()))- Computed arrays:
items.map(x => this.process(arr.slice(0, 2)))- Imported/external arrays
This is likely sufficient for most use cases, but consider adding a comment explaining this scope limitation.
packages/bubble-runtime/src/extraction/BubbleParser.test.ts (1)
432-455: Consider edge case handling in extractClass helper.The
extractClasshelper extracts a single class from the fixture by finding the start and end lines. However, the logic at lines 442-450 assumes:
- Each class starts with
export class- The next class declaration marks the end of the current class
- The last class extends to the end of the file
This works for the current fixture structure, but could fail if:
- A class contains a nested class
- Comments contain the text "export class"
- The class has complex formatting
Since this is test-only code and works for the current fixture, this is acceptable. Consider adding a comment noting these assumptions.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/bubble-runtime/src/extraction/BubbleParser.test.tspackages/bubble-runtime/src/extraction/BubbleParser.tspackages/bubble-runtime/tests/fixtures/index.tspackages/bubble-runtime/tests/fixtures/promise-all-patterns.ts
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: bubblelabai/BubbleLab PR: 0
File: .cursor/rules/api.mdc:0-0
Timestamp: 2025-12-19T03:16:48.793Z
Learning: Applies to **/bubblelab-api/**/*.test.{js,ts} : Use `pnpm bun test` command to run backend tests to ensure setup files are properly loaded
Learnt from: CR
Repo: bubblelabai/BubbleLab PR: 0
File: .cursor/rules/api.mdc:0-0
Timestamp: 2025-12-19T03:16:48.793Z
Learning: Applies to **/bubblelab-api/**/*.test.{js,ts} : Reference webhook.test as the example for how backend tests should be written
📚 Learning: 2025-12-19T03:17:06.817Z
Learnt from: CR
Repo: bubblelabai/BubbleLab PR: 0
File: .cursor/rules/bundling.mdc:0-0
Timestamp: 2025-12-19T03:17:06.817Z
Learning: Applies to packages/bubble-core/src/index.ts : All new types and classes added to bubble-core must be exported from `packages/bubble-core/src/index.ts` to ensure they are included in the generated bundle
Applied to files:
packages/bubble-runtime/tests/fixtures/index.tspackages/bubble-runtime/tests/fixtures/promise-all-patterns.tspackages/bubble-runtime/src/extraction/BubbleParser.test.ts
📚 Learning: 2025-12-19T03:17:06.817Z
Learnt from: CR
Repo: bubblelabai/BubbleLab PR: 0
File: .cursor/rules/bundling.mdc:0-0
Timestamp: 2025-12-19T03:17:06.817Z
Learning: When adding new types to `bubblelab/shared-schemas`, they are automatically included in the bundle without requiring manual configuration changes
Applied to files:
packages/bubble-runtime/tests/fixtures/index.ts
📚 Learning: 2025-12-19T03:17:06.817Z
Learnt from: CR
Repo: bubblelabai/BubbleLab PR: 0
File: .cursor/rules/bundling.mdc:0-0
Timestamp: 2025-12-19T03:17:06.817Z
Learning: Applies to packages/bubble-core/dist/**/*.d.ts : The type bundling system for Monaco Editor must process TypeScript declaration files (.d.ts) and inline all dependencies into a single self-contained bundle in `packages/bubble-core/dist/bubble-bundle.d.ts`
Applied to files:
packages/bubble-runtime/tests/fixtures/index.tspackages/bubble-runtime/tests/fixtures/promise-all-patterns.tspackages/bubble-runtime/src/extraction/BubbleParser.test.ts
📚 Learning: 2025-12-22T09:55:47.864Z
Learnt from: CR
Repo: bubblelabai/BubbleLab PR: 0
File: .cursor/rules/bubblelab.mdc:0-0
Timestamp: 2025-12-22T09:55:47.864Z
Learning: Applies to packages/bubble-runtime/src/extraction/README.md : Refer to packages/bubble-runtime/src/extraction/README.md for information about bubble parsing, dependency graphs, and per-invocation cloning
Applied to files:
packages/bubble-runtime/src/extraction/BubbleParser.tspackages/bubble-runtime/src/extraction/BubbleParser.test.ts
📚 Learning: 2025-12-19T03:16:48.793Z
Learnt from: CR
Repo: bubblelabai/BubbleLab PR: 0
File: .cursor/rules/api.mdc:0-0
Timestamp: 2025-12-19T03:16:48.793Z
Learning: Applies to **/bubblelab-api/**/*.test.{js,ts} : Reference webhook.test as the example for how backend tests should be written
Applied to files:
packages/bubble-runtime/tests/fixtures/promise-all-patterns.tspackages/bubble-runtime/src/extraction/BubbleParser.test.ts
📚 Learning: 2025-12-19T03:16:48.793Z
Learnt from: CR
Repo: bubblelabai/BubbleLab PR: 0
File: .cursor/rules/api.mdc:0-0
Timestamp: 2025-12-19T03:16:48.793Z
Learning: Applies to **/bubblelab-api/**/*.test.{js,ts} : Use `pnpm bun test` command to run backend tests to ensure setup files are properly loaded
Applied to files:
packages/bubble-runtime/tests/fixtures/promise-all-patterns.tspackages/bubble-runtime/src/extraction/BubbleParser.test.ts
📚 Learning: 2025-12-19T03:17:06.817Z
Learnt from: CR
Repo: bubblelabai/BubbleLab PR: 0
File: .cursor/rules/bundling.mdc:0-0
Timestamp: 2025-12-19T03:17:06.817Z
Learning: Applies to packages/bubble-core/dist/**/*.d.ts : All external imports (except relative imports) must be removed from the bundled output to prevent circular processing and external dependency issues
Applied to files:
packages/bubble-runtime/tests/fixtures/promise-all-patterns.tspackages/bubble-runtime/src/extraction/BubbleParser.test.ts
📚 Learning: 2025-12-22T09:55:47.864Z
Learnt from: CR
Repo: bubblelabai/BubbleLab PR: 0
File: .cursor/rules/bubblelab.mdc:0-0
Timestamp: 2025-12-22T09:55:47.864Z
Learning: Applies to packages/bubble-shared-schemas/src/bubbleflow-generation-prompts.ts : Refer to packages/bubble-shared-schemas/src/bubbleflow-generation-prompts.ts for documentation on how bubble flow is supposed to be generated
Applied to files:
packages/bubble-runtime/tests/fixtures/promise-all-patterns.tspackages/bubble-runtime/src/extraction/BubbleParser.test.ts
🧬 Code graph analysis (2)
packages/bubble-runtime/tests/fixtures/promise-all-patterns.ts (2)
packages/bubble-core/src/index.ts (1)
BubbleFlow(19-19)packages/bubble-shared-schemas/src/trigger.ts (1)
WebhookEvent(64-66)
packages/bubble-runtime/src/extraction/BubbleParser.test.ts (4)
packages/bubble-core/src/bubble-factory.ts (1)
BubbleFactory(61-810)packages/bubble-core/src/index.ts (1)
BubbleFactory(118-118)packages/bubble-runtime/tests/fixtures/index.ts (1)
getFixture(90-92)packages/bubble-runtime/src/extraction/BubbleParser.ts (1)
BubbleParser(50-4407)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (5)
packages/bubble-runtime/src/extraction/BubbleParser.ts (1)
3843-3934: LGTM! Well-structured refactoring with expanded functionality.The renaming from
findArrayPushCallstofindArrayElementsappropriately reflects the expanded scope. The implementation correctly handles both.push()and.map()patterns, properly using scope resolution to match variable IDs and avoid false positives from same-named variables in different scopes.packages/bubble-runtime/tests/fixtures/promise-all-patterns.ts (1)
1-112: Excellent test fixture with comprehensive coverage.This fixture file provides thorough coverage of Promise.all patterns:
- Direct array literals
- Array with
.push()calls.map()with expression body.map()with block body.map()with variable arraysEach test case is clear, focused, and representative of real-world usage patterns. The consistent structure (extending BubbleFlow, returning Output, using simple helper methods) makes them easy to understand and maintain.
packages/bubble-runtime/tests/fixtures/index.ts (1)
71-71: LGTM! Fixture properly registered.The new fixture is correctly added to the registry, following the established pattern.
packages/bubble-runtime/src/extraction/BubbleParser.test.ts (2)
457-632: Comprehensive test coverage validates the fix.The test suite thoroughly exercises all Promise.all patterns:
- Direct array literals
.push()patterns.map()with expression bodies.map()with block bodies.map()with variable arraysEach test appropriately verifies:
- A
parallel_executionnode is created- The correct number of children are present
- Children are of the expected types (
function_callortransformation_function)The use of
toBeGreaterThanOrEqualfor the variable array test (line 624) is appropriate since the array size is determined by fixture content rather than being explicitly 3.
634-667: Good defensive testing with relaxed assertions for edge cases.The final test using the existing
bubble-inside-promisefixture is valuable as a regression test. The relaxed assertions (line 665:toBeGreaterThan(0)instead of exact count) are appropriate here since the test comment notes the pattern might not be fully detected by the current implementation.This test ensures the parser doesn't break on existing fixtures while the new tests validate the enhanced functionality.
3 participants
Summary
All Promise.all patterns now correctly visualize multiple parallel nodes
Related Issues
#241
Type of Change
Checklist
pnpm checkand all tests passScreenshots (if applicable)
Additional Context
BubbleParser.ts: Merged array element extraction logic into unified methodBubbleParser.test.ts: Added comprehensive tests for all Promise.all patternspromise-all-patterns.ts: Created fixture file with 5 test casesSummary by CodeRabbit
Tests
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.