Merged
Conversation
📝 WalkthroughWalkthroughIntroduces a comprehensive new "AssembledBubble" service integration for managing personnel, activities, time off, and organizational entities. The implementation includes zod-based schema validation, API request utilities with authentication and error handling, credential type registration, and an integration test flow executing multiple sequential operations with conditional branching. Version bumps applied across all packages. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying bubblelab-documentation with
|
| Latest commit: |
2ab28ba
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8f74baa5.bubblelab-documentation.pages.dev |
| Branch Preview URL: | https://assembled.bubblelab-documentation.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/bubble-core/src/bubbles/service-bubble/assembled/assembled.integration.flow.ts`:
- Line 87: The output currently emits the full person payload via
JSON.stringify(createResult.data?.person); change the integration output to only
include non-sensitive identifiers (e.g., person.id, person.uuid, or
person.externalId) instead of the whole object. Locate the template string that
references createResult.data?.person in assembled.integration.flow.ts and
replace the JSON.stringify usage with a concise identifier(s) (for example
`createResult.data?.person.id`) so PII like name/email is not included in flow
output or logs.
In `@packages/bubble-core/src/bubbles/service-bubble/assembled/assembled.ts`:
- Around line 192-193: The path templates in assembled.ts interpolate raw IDs
(e.g., `/people/${params.person_id}` and the other occurrences at the same file
around the other reported spots) which can break routing for IDs containing
reserved URL characters; update those interpolations to use encodeURIComponent
on the ID values before inserting them into the path (e.g., replace direct uses
of params.person_id and the other raw id variables with encoded values) so all
requests build safe paths; ensure you update every occurrence mentioned (the
instances at the earlier `/people/${params.person_id}` and the two other
interpolations noted) and run tests or a quick manual check to confirm endpoints
remain correct.
- Line 315: The current guard if (params.allow_conflicts) body.allow_conflicts =
params.allow_conflicts; drops explicit false; change the check to test for
presence instead of truthiness (e.g., if
(params.hasOwnProperty('allow_conflicts')) or if (typeof params.allow_conflicts
!== 'undefined') ) and then assign body.allow_conflicts = params.allow_conflicts
so false is preserved; update the logic in assembled.ts around the
params.allow_conflicts → body.allow_conflicts assignment.
In `@packages/bubble-shared-schemas/src/bubble-definition-schema.ts`:
- Line 77: Add the new ASSEMBLED_CRED mapping and then run the core build to
regenerate types: ensure CredentialType.ASSEMBLED_CRED is present in the enum
and that the empty config entry in bubble-definition-schema.ts
([CredentialType.ASSEMBLED_CRED]: {}) is committed, then run pnpm build:core to
rebuild the shared schema types so downstream packages pick up the new
credential mapping.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
apps/bubble-studio/src/pages/CredentialsPage.tsxpackages/bubble-core/package.jsonpackages/bubble-core/src/bubble-factory.tspackages/bubble-core/src/bubbles/service-bubble/assembled/assembled.integration.flow.tspackages/bubble-core/src/bubbles/service-bubble/assembled/assembled.schema.tspackages/bubble-core/src/bubbles/service-bubble/assembled/assembled.tspackages/bubble-core/src/bubbles/service-bubble/assembled/assembled.utils.tspackages/bubble-core/src/bubbles/service-bubble/assembled/index.tspackages/bubble-core/src/index.tspackages/bubble-runtime/package.jsonpackages/bubble-scope-manager/package.jsonpackages/bubble-shared-schemas/package.jsonpackages/bubble-shared-schemas/src/bubble-definition-schema.tspackages/bubble-shared-schemas/src/capability-schema.tspackages/bubble-shared-schemas/src/credential-schema.tspackages/bubble-shared-schemas/src/types.tspackages/create-bubblelab-app/package.jsonpackages/create-bubblelab-app/templates/basic/package.jsonpackages/create-bubblelab-app/templates/reddit-scraper/package.json
| operation: 'create_person', | ||
| success: createResult.success, | ||
| details: createResult.success | ||
| ? `Created person: ${JSON.stringify(createResult.data?.person)}` |
There was a problem hiding this comment.
Avoid emitting full person payload in integration output details.
Line 87 includes JSON.stringify(createResult.data?.person), which can leak PII (for example email/name) through flow output and logs. Return only non-sensitive identifiers.
🔧 Proposed fix
results.push({
operation: 'create_person',
success: createResult.success,
details: createResult.success
- ? `Created person: ${JSON.stringify(createResult.data?.person)}`
+ ? `Created person ID: ${((createResult.data?.person as Record<string, unknown>)?.id as string | undefined) ?? 'unknown'}`
: createResult.error,
});📝 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.
Suggested change
| ? `Created person: ${JSON.stringify(createResult.data?.person)}` | |
| results.push({ | |
| operation: 'create_person', | |
| success: createResult.success, | |
| details: createResult.success | |
| ? `Created person ID: ${((createResult.data?.person as Record<string, unknown>)?.id as string | undefined) ?? 'unknown'}` | |
| : createResult.error, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/bubble-core/src/bubbles/service-bubble/assembled/assembled.integration.flow.ts`
at line 87, The output currently emits the full person payload via
JSON.stringify(createResult.data?.person); change the integration output to only
include non-sensitive identifiers (e.g., person.id, person.uuid, or
person.externalId) instead of the whole object. Locate the template string that
references createResult.data?.person in assembled.integration.flow.ts and
replace the JSON.stringify usage with a concise identifier(s) (for example
`createResult.data?.person.id`) so PII like name/email is not included in flow
output or logs.
Comment on lines
+192
to
+193
| path: `/people/${params.person_id}`, | ||
| apiKey, |
There was a problem hiding this comment.
Encode path IDs before URL interpolation.
Line 192, Line 254, and Line 423 interpolate raw IDs into path segments. Reserved characters in IDs (/, ?, #) can change routing and hit the wrong endpoint.
🔧 Proposed fix
- path: `/people/${params.person_id}`,
+ path: `/people/${encodeURIComponent(params.person_id)}`,
...
- path: `/people/${params.person_id}`,
+ path: `/people/${encodeURIComponent(params.person_id)}`,
...
- path: `/time_off/${params.time_off_id}/cancel`,
+ path: `/time_off/${encodeURIComponent(params.time_off_id)}/cancel`,Also applies to: 254-255, 423-424
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bubble-core/src/bubbles/service-bubble/assembled/assembled.ts`
around lines 192 - 193, The path templates in assembled.ts interpolate raw IDs
(e.g., `/people/${params.person_id}` and the other occurrences at the same file
around the other reported spots) which can break routing for IDs containing
reserved URL characters; update those interpolations to use encodeURIComponent
on the ID values before inserting them into the path (e.g., replace direct uses
of params.person_id and the other raw id variables with encoded values) so all
requests build safe paths; ensure you update every occurrence mentioned (the
instances at the earlier `/people/${params.person_id}` and the two other
interpolations noted) and run tests or a quick manual check to confirm endpoints
remain correct.
| }; | ||
| if (params.channels) body.channels = params.channels; | ||
| if (params.description) body.description = params.description; | ||
| if (params.allow_conflicts) body.allow_conflicts = params.allow_conflicts; |
There was a problem hiding this comment.
allow_conflicts=false is currently dropped from the request.
Line 315 uses a truthy guard, so explicit false is omitted. This can change API behavior if server defaults differ.
🔧 Proposed fix
- if (params.allow_conflicts) body.allow_conflicts = params.allow_conflicts;
+ if (params.allow_conflicts !== undefined) {
+ body.allow_conflicts = params.allow_conflicts;
+ }📝 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.
Suggested change
| if (params.allow_conflicts) body.allow_conflicts = params.allow_conflicts; | |
| if (params.allow_conflicts !== undefined) { | |
| body.allow_conflicts = params.allow_conflicts; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bubble-core/src/bubbles/service-bubble/assembled/assembled.ts` at
line 315, The current guard if (params.allow_conflicts) body.allow_conflicts =
params.allow_conflicts; drops explicit false; change the check to test for
presence instead of truthiness (e.g., if
(params.hasOwnProperty('allow_conflicts')) or if (typeof params.allow_conflicts
!== 'undefined') ) and then assign body.allow_conflicts = params.allow_conflicts
so false is preserved; update the logic in assembled.ts around the
params.allow_conflicts → body.allow_conflicts assignment.
| [CredentialType.ATTIO_CRED]: {}, | ||
| [CredentialType.HUBSPOT_CRED]: {}, | ||
| [CredentialType.SORTLY_API_KEY]: {}, | ||
| [CredentialType.ASSEMBLED_CRED]: {}, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify ASSEMBLED_CRED is defined in CredentialType enum and related files
echo "=== Checking CredentialType enum for ASSEMBLED_CRED ==="
rg -n "ASSEMBLED_CRED" --type=ts -C2 packages/bubble-shared-schemas/src/types.ts
echo ""
echo "=== Checking credential-schema.ts for ASSEMBLED_CRED mappings ==="
rg -n "ASSEMBLED_CRED" --type=ts -C2 packages/bubble-shared-schemas/src/credential-schema.ts
echo ""
echo "=== Checking bubble-definition-schema.ts for context ==="
rg -n "ASSEMBLED_CRED\|CREDENTIAL_CONFIGURATION_MAP" --type=ts -A5 -B5 packages/bubble-shared-schemas/src/bubble-definition-schema.ts | head -60Repository: bubblelabai/BubbleLab
Length of output: 926
🏁 Script executed:
# Verify the context around line 77 in bubble-definition-schema.ts to see the full CREDENTIAL_CONFIGURATION_MAP structure
sed -n '70,90p' packages/bubble-shared-schemas/src/bubble-definition-schema.tsRepository: bubblelabai/BubbleLab
Length of output: 790
LGTM! Remember to run pnpm build:core after this change.
The new ASSEMBLED_CRED entry follows the established pattern for credential configuration mappings. The empty config object is appropriate since Assembled doesn't require proxy or special configuration fields like some other credentials (e.g., AMAZON_CRED, LINKEDIN_CRED). Verification confirms ASSEMBLED_CRED is properly defined in the CredentialType enum and has corresponding credential schema mappings.
Since this is a shared schema modification in /packages/bubble-shared-schemas, remember to run pnpm build:core to regenerate the types.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bubble-shared-schemas/src/bubble-definition-schema.ts` at line 77,
Add the new ASSEMBLED_CRED mapping and then run the core build to regenerate
types: ensure CredentialType.ASSEMBLED_CRED is present in the enum and that the
empty config entry in bubble-definition-schema.ts
([CredentialType.ASSEMBLED_CRED]: {}) is committed, then run pnpm build:core to
rebuild the shared schema types so downstream packages pick up the new
credential mapping.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Related Issues
Type of Change
Checklist
pnpm checkand all tests passScreenshots (Required)
For New Bubble Integrations
.integration.flow.ts) covers all operationsAdditional Context
Summary by CodeRabbit
New Features
Chores