-
Notifications
You must be signed in to change notification settings - Fork 247
Improve performance adding integrations from editor sidebar #1828
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve performance adding integrations from editor sidebar #1828
Conversation
ad97287 to
9406650
Compare
| const createIntegrationLatte = defineLatteTool( | ||
| async ({ name, app: appName }, { workspace, user }) => { | ||
| const appResult = await getApp({ name: appName }) | ||
| const appResult = await getApp({ name: appName, withConfig: true }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@csansoon is this the one that needs config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use to get the tools here before, so yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup
| }): PromisedResult<ConfigurablePropWithRemoteOptions[]> { | ||
| try { | ||
| // TODO: Cache this on Redis to improve performance on tool retrieval | ||
| const { data: component } = await pipedream.components.retrieve(componentId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could also be cached in Redis to improve tool retrieval at request time. Not doing in this PR tho
| title: `Revert changes for "${oldDocumentPath}"`, | ||
| description: `Reverted changes of "${oldDocumentPath}" made in version ${changedCommit.title}`, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont know what happened with Prettier @geclos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no idea
de1ccb6 to
71471c8
Compare
| : undefined | ||
| const { data: pipedreamApp } = usePipedreamApp(appNameSlug) | ||
| const { data: pipedreamApp } = usePipedreamApp(appNameSlug, { | ||
| withConfig: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes the data retrieved from the integration provider (Pie...) leaner. They have a large property with a lot of JSON that we don't need in most cases when using listing tools.
|
|
||
| <PipedreamAppCard app={app} /> | ||
| {!isLoadingAppData && !hasTools ? ( | ||
| <Alert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the tradeoff with not fetching the N+1 integrations + tools all the time. We can't do the filtering by tool or trigger so there will be false positives in the UI so we let users know this integration does not have tools in this case. Sorry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uff... That sounds frustrating.
Pipedream actually had a filtering option in their sdk before updating it. They FORGOT to add it back in the new version and just created an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use that option when is ready in the SDK
| toolCount?: number | ||
| }) { | ||
| if (type === 'UnConnectedPipedreamApp') { | ||
| const tools = pluralize(toolCount ?? 0, 'available tool', 'available tools') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No counter of tools in the connect integration modal. This is the other tradeoff. Happy to do it because we get faster first listing of tools.
| }, | ||
| }) satisfies SearchableOptionItem<ToolType>, | ||
| ) | ||
| const availableApps: SearchableOptionItem<ToolType>[] = pipedreamApps.map( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we list all integrations before we filter out already connected integrations. No need. Users will see, for example, Slack on their already connected and they can connect from a new account, or they can click on connect in the non-connected integrations. This is better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this better? Please explain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a product decision. Is better because you can see always all the apps available even the ones you already have connected. This way you have 2 places to make the connection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That can add confusion to the user, dont you think?
| const { data: pipedreamData, isLoading: loadingComponents } = usePipedreamApp( | ||
| (integrationForTrigger as PipedreamIntegration | undefined)?.configuration | ||
| .appName, | ||
| { withConfig: true }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this fat trigger is needed for the configuration panel on the triggers modal
| const { data, isLoading } = usePipedreamApp(integration.configuration.appName) | ||
| const { data, isLoading } = usePipedreamApp( | ||
| integration.configuration.appName, | ||
| { withConfig: true }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this fat trigger is needed for the configuration panel on the triggers modal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FAT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| trigger: DocumentTrigger<DocumentTriggerType.Integration> | ||
| integration: PipedreamIntegration | ||
| component?: PipedreamComponent | ||
| component?: LightPipedreamComponent<T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When withConfig: false we have a new type without the fat JSON property
71471c8 to
8ed554f
Compare
8ed554f to
5243ce7
Compare
| { nameSlug: string; timestamp: Date }[] | ||
| >([]) | ||
|
|
||
| const { execute: searchApps, isPending: isSearching } = useLatitudeAction( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: not a fan of leaving actions within a component, I suggest we create a store for all of the actions for invalidating the cache for cleaner code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a one direction only action. No need to use a store
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it's a single instance it's fine, when it's time to DRY moving to stores is correct
| const handleInvalidate = async (nameSlug: string) => { | ||
| const [result, _error] = await invalidateApp({ nameSlug }) | ||
| if (result) { | ||
| setInvalidatedApps((prev) => [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could also go within the store, including the useEffects, and it would be cleaner than poluting a component with all this logic
| const { searchParams } = new URL(request.url) | ||
| const query = searchParams.get('query') || undefined | ||
| const cursor = searchParams.get('cursor') || undefined | ||
| const withTriggers = searchParams.get('withTriggers') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we removing here the trigger and tools?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the N+1 queries that we're not doing now when fetching the applications. We're not fetching all their triggers and tools
| } | ||
|
|
||
| // Light version of AppDto without configurableProps for reduced payload | ||
| export type LightPipedreamComponent<T extends PipedreamComponentType> = Omit< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Light is not comprehensible for me, would rename to more literal AppWithoutConfig
| @@ -1,5 +1,3 @@ | |||
| version: '3.9' | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be removed IMO, could potentially break the docker compose although I never use it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have in the other docker compose
https://github.com/latitude-dev/latitude-llm/blob/main/docker-compose.yml
And docker was complaining that version is legacy
| return getOrSet<CachedAppsResponse>(cacheKey, fetchApps, ONE_DAY_IN_SECONDS) | ||
| } | ||
|
|
||
| export type CachedAppResponse<C extends boolean> = C extends true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type is also used in the frontend store, why duplicate it? We can add it in constants to share
| type MaybeAppDto<C extends boolean> = C extends true | ||
| ? AppDto | ||
| : C extends false | ||
| ? LightAppDto | ||
| : never | ||
| type AppResponse<C extends boolean> = | ||
| | { data: MaybeAppDto<C>; ok: true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we just update AppDto to have tools and triggers optional? That way we will have to check for them in the UI and decide whether to show each UI depending on if the attribute is there or not.
Also, this way we don't have multiple types for the same thing, it is confusing: App, AppDto, LightAppDto, MaybeAppDto<C>...

What?
Some improvements on connection a integration (tool or trigger)
TODO