-
Notifications
You must be signed in to change notification settings - Fork 237
feat(compass-assistant): track opening and closing the drawer sections COMPASS-9884 #7372
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
Conversation
</DrawerActionsContext.Provider> | ||
<DrawerCurrentTabStateContext.Provider value={drawerCurrentTab}> | ||
<DrawerSetCurrentTabContext.Provider value={setDrawerCurrentTab}> | ||
<DrawerActionsContext.Provider value={drawerActions}> |
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.
The nesting here is getting a bit wild. Can/should we not combine some of it? Is there a better way? I did kinda just copy what we're already doing for isOpen.
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 group all actions together, but for state if we're putting it in one context, there would be no way to subscribe to changes to a single value instead of all of 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.
Hmm I guess it is OK then.
} | ||
}, | ||
[ensureOptInAndSend, setMessages] | ||
[ensureOptInAndSend, setMessages, track] |
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.
drive-by
07f8296
to
0fd8470
Compare
0fd8470
to
20ac43b
Compare
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.
Pull Request Overview
This PR adds telemetry tracking for drawer section open/close events in the Compass application. It implements callbacks to track when users open or close drawer sections, either by switching tabs in the drawer toolbar or opening/closing the drawer itself.
- Add telemetry events for tracking drawer section interactions
- Implement callback props in drawer components to handle open/close events
- Update tests to verify the new tracking functionality
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
packages/compass/src/app/components/home.tsx | Wire up drawer section tracking callbacks to telemetry |
packages/compass-web/src/entrypoint.tsx | Wire up drawer section tracking callbacks to telemetry |
packages/compass-telemetry/src/telemetry-events.ts | Define new telemetry event types for drawer section interactions |
packages/compass-components/src/components/drawer-portal.tsx | Implement drawer section tracking logic and callback props |
packages/compass-components/src/components/drawer-portal.spec.tsx | Add tests for drawer section tracking functionality |
packages/compass-components/src/components/compass-components-provider.tsx | Pass through drawer section tracking props |
packages/compass-assistant/src/components/assistant-chat.tsx | Add track dependency to useCallback hook |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
useEffect(() => { | ||
const currentTab = | ||
drawerToolbarContext.getActiveDrawerContent()?.id ?? null; | ||
|
||
currentTabSetter(currentTab); | ||
}, [drawerToolbarContext, currentTabSetter]); | ||
|
Copilot
AI
Sep 25, 2025
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 useEffect will run on every change to drawerToolbarContext
, but it should only run when the active drawer content actually changes. Consider extracting drawerToolbarContext.getActiveDrawerContent()?.id
to the dependency array or using a useMemo to optimize this effect.
useEffect(() => { | |
const currentTab = | |
drawerToolbarContext.getActiveDrawerContent()?.id ?? null; | |
currentTabSetter(currentTab); | |
}, [drawerToolbarContext, currentTabSetter]); | |
const activeDrawerContentId = useMemo( | |
() => drawerToolbarContext.getActiveDrawerContent()?.id ?? null, | |
[drawerToolbarContext] | |
); | |
useEffect(() => { | |
currentTabSetter(activeDrawerContentId); | |
}, [activeDrawerContentId, currentTabSetter]); |
Copilot uses AI. Check for mistakes.
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'm guessing we wouldn't want this because we want open/close even if it is the same tab?
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.
There isn't a value we can check. There's a method. 🤷
This adds open / close tracking for all drawer sections whenever you switch between sections (like via the toolbar or hooks) or when you open/close the drawer or when sections disappear.