fix: provide worspaceId to AnalyticsInitializer #7642
Conversation
github-actions
bot
added
area/frontend
area/platform
airbyte-webapp/src/App.tsx
Outdated
| @@ -31,7 +31,10 @@ import { | |||
| function useCustomerIdProvider() { | |||
| const workspace = useCurrentWorkspace(); | |||
|
|
|||
| return workspace.customerId; | |||
| return { | |||
There was a problem hiding this comment.
As we have changed a bit logic to more declarative way, there is no need to change customerIdProvider anymore.
| @@ -1,7 +1,10 @@ | |||
| import { SegmentAnalytics } from "./types"; | |||
|
|
|||
| export class AnalyticsService { | |||
| constructor(private userId?: string, private version?: string) {} | |||
| constructor( | |||
| private context: Record<string, unknown>, | |||
There was a problem hiding this comment.
I think something like additionalContext suits better.
| ); | ||
|
|
||
| const handleAddContextProps = (props: Record<string, unknown>) => { | ||
| return { |
There was a problem hiding this comment.
I don't see where it updates ctx
| }: { | ||
| children: React.ReactNode; | ||
| version?: string; | ||
| userId?: string; | ||
| context?: Record<string, unknown>; |
There was a problem hiding this comment.
we don't want to set all context at once. This prop makes it a bit confusing that you are providing context, however it provides additional context ( or initAnalyticsContext )
|
|
||
| return { | ||
| [key]: ctx[key], | ||
| ...result, |
There was a problem hiding this comment.
As you are building from new object, there is no need to spread here and just result[key] could be used.
| }; | ||
|
|
||
| const handleRemoveContextProps = (props: string[]) => { | ||
| const newCtx = Object.keys(ctx).reduce((result, key) => { |
There was a problem hiding this comment.
I think something like
Object.entries()
...
if(!props.includes(key)){
result[key]=value;
}
return result;
is easier for understanding
| const { workspaceId, customerId } = useCurrentWorkspace(); | ||
| const { user } = useAuthService(); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
I believe it's better to add it as a separate hook, so it could be used as.
useProvideAnalyticsProp({workspaceId, customerId} // or some other better name
so it will add and remove from context where necessary.
| const { user } = useAuthService(); | ||
|
|
||
| useEffect(() => { | ||
| ctx.setContext({ workspaceId, userId: user?.userId, customerId }); |
There was a problem hiding this comment.
We have useCurrentUser that is always present.
* fix: provide worspaceId to AnalyticsInitializer * Refactor Analytics for more independent approach * Speed up initial load for cloud webapp Co-authored-by: Artem Astapenko <jamakase54@gmail.com>
Closes #7596